[16.0][IMP] membership_extension & _variable_period: Make sure necessary fields are always truthy #157
Conversation
carmenbianca
force-pushed
the
16.0-add-constrains
branch
3 times, most recently
from
d475f3e
to
3f8afd4
Compare
There was a problem hiding this comment.
Hi @carmenbianca I'm ok with your suggestions. Just a couple of comments:
| for record in self: | ||
| if record.membership: | ||
| if not record.membership_date_from or not record.membership_date_to: |
There was a problem hiding this comment.
| for record in self: | |
| if record.membership: | |
| if not record.membership_date_from or not record.membership_date_to: | |
| if self.filtered(lambda x: x.membership and (not x.membership_date_from or not x.membership_date_to): |
| for record in self: | ||
| if record.membership: | ||
| if record.membership_type == "fixed": | ||
| return super(ProductTemplate, record)._check_membership_dates() |
There was a problem hiding this comment.
You're breaking your loop here. Maybe what you want to do is:
| for record in self: | |
| if record.membership: | |
| if record.membership_type == "fixed": | |
| return super(ProductTemplate, record)._check_membership_dates() | |
| return super(ProductTemplate, self.filtered(lambda x: x.membership_type == "fixed"))._check_membership_dates() |
…elds are always truthy This is an additional sanity check to make sure that fields that are expected to be available are actually available. The situation here is a little tricky. Especially as pertains date_from and date_to; these fields are `required=True` nowhere. Not on the product, not on the membership invoice, and not on the membership line. However, they _are_ set as required in the product and membership line views. If these values are ever empty, it's a sure sign that something went wrong somewhere, because the user should not be able to reach such a state via the UI. One sure way to reach that state was via the demo data, however, which had no start and end dates. This is now fixed. The situation for the variable period fields is similar. These fields should never be empty when the membership type is variable, and it's not typically possible to empty these values via the UI. For convenience's sake, I simply set them to required an sich, with a default value. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
carmenbianca
force-pushed
the
16.0-add-constrains
branch
from
3f8afd4
to
315bdba
Compare
|
@chienandalu I've implemented and pushed your suggestions. Thank you for the review. |
| cls.product_1.membership = True | ||
| cls.product_1.write( | ||
| { | ||
| "membership_date_from": "2023-01-01", |
There was a problem hiding this comment.
Why do you need to change this?
There was a problem hiding this comment.
@pedrobaeza Because the new constraint won't let a membership product have no start/end dates.
@api.constrains("membership_date_from", "membership_date_to", "membership")
def _check_membership_dates(self):
if self.filtered(
lambda record: record.membership
and (not record.membership_date_from or not record.membership_date_to)
):
raise ValidationError(
_("A membership product must have a start date and an end date.")
)(Interestingly, the write function is sensitive to the order of key-value pairs. If "membership": True appears before the other items, the constraint complains. Not sure why that happens.)
There was a problem hiding this comment.
It's weird, as in other places, you are removing such fields, but it's not a big deal.
/ocabot merge patch
|
/ocabot merge patch |
|
Hey, thanks for contributing! Proceeding to merge this for you. |
|
Congratulations, your PR was merged at 3ee9de5. Thanks a lot for contributing to OCA. ❤️ |
This is an additional sanity check to make sure that fields that are expected to be available are actually available.
The situation here is a little tricky. Especially as pertains date_from and date_to; these fields are
required=Truenowhere. Not on the product, not on the membership invoice, and not on the membership line. However, they are set as required in the product and membership line views. If these values are ever empty, it's a sure sign that something went wrong somewhere, because the user should not be able to reach such a state via the UI.One sure way to reach that state was via the demo data, however, which had no start and end dates. This is now fixed.
The situation for the variable period fields is similar. These fields should never be empty when the membership type is variable, and it's not typically possible to empty these values via the UI. For convenience's sake, I simply set them to required an sich, with a default value.