Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[17.0][MIG] partner_event: Migration to version 17.0 #382

Merged
merged 40 commits into from
Jul 18, 2024

Conversation

juanjosesegui-tecnativa
Copy link

@Tecnativa TT49923

antespi and others added 30 commits July 3, 2024 09:23
These fields were stored. That usually is good, but if you add partners to an event from the wizard, it means each `res.partner` record gets its `registration_count` field updated.

When you update any record, fields `write_uid` and `write_date` are updated too.

Now imagine you want to add 4000 partners to an event. That would take time. If in the mean time any other user updates the `res.partner` record (along with their `write_UID` and `WRITE_DATE` fields), you would get this error:

```
openerp.sql_db: bad query: UPDATE "res_partner" SET "registration_count"=3,"write_uid"=5,"write_date"=(now() at time zone 'UTC') WHERE id IN (25578)
Traceback (most recent call last):
  File "/opt/odoo/common/openerp/v8/openerp/sql_db.py", line 234, in execute
    res = self._obj.execute(query, params)
TransactionRollbackError: could not serialize access due to concurrent update
CONTEXT:  SQL statement "SELECT 1 FROM ONLY "public"."res_users" x WHERE "id" OPERATOR(pg_catalog.=) $1 FOR KEY SHARE OF x"
```

By removing the `store=True` parameter, you simply avoid those concurrent updates and do not block other users.

But this changes the database structure, so remember to update your database!
If adding 1000 partners, we save now 999 queries.

(cherry picked from commit 89853ef)
Currently translated at 100.0% (24 of 24 strings)

Translation: event-12.0/event-12.0-partner_event
Translate-URL: https://translation.odoo-community.org/projects/event-12-0/event-12-0-partner_event/es/
…oint

For speeding up the process. Specially the removal of the savepoints
makes that Postgres don't go slow as hell. Check
https://www.cybertec-postgresql.com/en/subtransactions-and-performance-in-postgresql/
for some rationale. The problem happens even releasing the previous
savepoints.

As this module is very old, the reason for doing it this way and skip
registrations with error is not fully known, but the view mentions about
registration duplication, so let's avoid it and handle later other
possible exceptions.

TT26694
When we confirm a sale order, the registrations are precreated without
the email info, so we were missing the chance to get the info for the
partner attendant.

Now we compute it on the write as well.

TT45149
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: event-16.0/event-16.0-partner_event
Translate-URL: https://translation.odoo-community.org/projects/event-16-0/event-16-0-partner_event/
Currently translated at 100.0% (23 of 23 strings)

Translation: event-16.0/event-16.0-partner_event
Translate-URL: https://translation.odoo-community.org/projects/event-16-0/event-16-0-partner_event/es/
@pedrobaeza
Copy link
Member

/ocabot migration partner_event

string="Attendances", compute="_compute_registration_count", store=False
)

@api.depends("event_registration_ids")
Copy link
Member

@pedrobaeza pedrobaeza Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of calling invalidate_recordset() in tests (which is somehow making tricks), you must add here in this depends the subfields (like event_registration_ids.state) for triggering the recompute automatically, as this is also going to provoke errors on its usage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

The tests pass perfectly with the change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is not correct. You have to add both, event_registration_ids and event_registration_ids.state, and any other field that can impact, and remove the invalidate_recordset calls in the tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have applied the changes and everything works correctly.

@juanjosesegui-tecnativa juanjosesegui-tecnativa force-pushed the 17.0-mig-partner_event branch 2 times, most recently from 8daab18 to 0db6580 Compare July 11, 2024 08:21
@pedrobaeza
Copy link
Member

/ocabot migration partner_event

@OCA-git-bot OCA-git-bot added this to the 17.0 milestone Jul 11, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request Jul 11, 2024
21 tasks
@pedrobaeza pedrobaeza requested a review from chienandalu July 11, 2024 16:09
Copy link

@carlos-lopez-tecnativa carlos-lopez-tecnativa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks :)

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 17.0-ocabot-merge-pr-382-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 36fc1ea into OCA:17.0 Jul 18, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at f2c791c. Thanks a lot for contributing to OCA. ❤️

@pedrobaeza pedrobaeza deleted the 17.0-mig-partner_event branch July 18, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.