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

[12.0][FIX] event_registration_multi_qty: Force quantity despite default #220

Merged

Conversation

joao-p-marques
Copy link
Member

@joao-p-marques joao-p-marques commented May 31, 2021

After odoo/odoo@a4d50b4, the passed value is not taken into account when a default value is set on the field.
This forces the update to the quantity.

cc @Tecnativa @chienandalu

After odoo/odoo@a4d50b4, the passed value is not taken into account when a default value is set on the field.
This adds the option of passing the desired value through the context and force that update.
@joao-p-marques joao-p-marques force-pushed the 12.0-fix-event_registration_multi_qty branch from e679684 to 1248db7 Compare June 1, 2021 05:47
@joao-p-marques
Copy link
Member Author

@pedrobaeza @victoralmau can you review this, please?

@pedrobaeza pedrobaeza added this to the 12.0 milestone Jun 1, 2021
@pedrobaeza
Copy link
Member

@chienandalu good for you?

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

One thing before merging: is this field in core? If so, please add tests. If not, you should put an explanation as comment.

@joao-p-marques
Copy link
Member Author

No, the field is added in this module. Is the current comment enough to explain it?

@pedrobaeza
Copy link
Member

Why then you check about if "qty" in registration:? If it's a record, it will always be True.

@chienandalu
Copy link
Member

chienandalu commented Jun 1, 2021

It's not a record, but a dictionary. Look: https://github.com/odoo/odoo/blob/a4d50b468c8f37155048e2c7bb0c816f45d27a95/addons/event/models/event.py#L439-L442

@joao-p-marques Maybe a link to the original method would ease the understanding of this change

@pedrobaeza
Copy link
Member

OK, the variable name is very bad (it should be registration_vals or so). And what about the test exercising this?

@joao-p-marques
Copy link
Member Author

the variable name is very bad

Yes, but it comes from core 🤷‍♂️

And what about the test exercising this?

Well, you said above to add tests if it the field came from core, which it does not. Do you still think it is worth it?

@pedrobaeza
Copy link
Member

Yes, I didn't express myself correctly. I mean that it comes with the dependency chain, not in extra module and this is a hack for avoiding a glue module. If this problem has arisen, a regression test would avoid to appear again in the future. Is it very complicated?

@joao-p-marques
Copy link
Member Author

Ok. Well, I guess I can add a simple test that calls the function and asserts the value is actually changed

@pedrobaeza
Copy link
Member

I'm afraid that doesn't serve. Tests should check real flows, not direct function calls. If not, in an upper version or future code revision, implementation may change, but the test will be correct.

@joao-p-marques
Copy link
Member Author

Well, then maybe @chienandalu can shed some light into what an appropriate test would be. Quite honestly, I didn't find this issue in UI, but in integration tests with another module, so that was the case I tested...

@chienandalu
Copy link
Member

The problem here is that although _prepare_attendee_values is defined in event is only used in core with event_sale and website_event_sale. I don't see how can we test it without adding one of those dependencies. That's why this error raised in a customer custom code test, which has all these dependencies joined althogether

@pedrobaeza
Copy link
Member

Ok, let's assume this and go:

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 12.0-ocabot-merge-pr-220-by-pedrobaeza-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit da855ae into OCA:12.0 Jun 1, 2021
@OCA-git-bot
Copy link
Contributor

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

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.

None yet

5 participants