-
-
Notifications
You must be signed in to change notification settings - Fork 112
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
[MIG] membership_variable_period: Migration to 10.0 #32
[MIG] membership_variable_period: Migration to 10.0 #32
Conversation
ea6cd9b
to
4a15a6b
Compare
Please, explain usage or how to review |
@luismontalba From the readme: 😊
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cubells please review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few changes to approve
invoice.date_invoice or fields.Date.today()) | ||
date_to = (product.product_tmpl_id._get_next_date(date_from) - | ||
def _prepare_membership_line(self, invoice, product, price_unit, line_id, | ||
qty=1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It must be qty=1.0
if later you call the method passing a float.
<field name="membership_type"/> | ||
<label for="membership_interval_qty" | ||
string="Interval" | ||
attrs="{'invisible': [('membership_type', '!=', 'variable')]}"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to add parameter t-translation="off"
in order to avoid attributes be translated, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't the second values of the selection tuples the ones being translated (like so: https://github.com/Tecnativa/vertical-association/blob/19ea87acbf6bb4adae7c1f522fa97ddc192d4065/membership_variable_period/models/product_template.py#L39)
Anyway, isn't t-translation
only valid for qweb templates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's possible @chienandalu . I have not tested it.
But anyway I recreated the translations files for that module and these strings:
msgid "{'invisible': [('membership_type', '!=', 'fixed')]}"
and these
msgid ""
"{'required': [('membership_type', '=', 'fixed'), ('membership', '=', True)],"
" 'invisible': [('membership_type', '!=', 'fixed')]}"
msgid "{'invisible': [('membership_type', '!=', 'fixed')]}"
and these:
msgid ""
+"{'required': [('membership_type', '=', 'fixed'), ('membership', '=', True)],"
+" 'invisible': [('membership_type', '!=', 'fixed')]}"
are not created.
So you have to avoid to be translated or you have an incorrect po files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cubells I see now. Fixed in the .po files. Weird thing anyway. I guess it's been there from long time ago. I tried to export the module's translations and got it right.
attrs="{'invisible': [('membership_type', '!=', 'variable')]}"/> | ||
<div attrs="{'invisible': [('membership_type', '!=', 'variable')]}"> | ||
<field name="membership_interval_qty" | ||
attrs="{'required': [('membership_type', '=', 'variable'), ('membership', '=', True)]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
class="oe_inline" | ||
nolabel="1"/> | ||
<field name="membership_interval_unit" | ||
attrs="{'required': [('membership_type', '=', 'variable'), ('membership', '=', True)]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
88cc478
to
19ea87a
Compare
19ea87a
to
2b025a6
Compare
@cubells please answer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
LGTM
Code review and test
Please squash for merging |
b0522db
to
1f96acb
Compare
@pedrobaeza Done |
Depends on:
cc @Tecnativa