-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
[16.0][MIG] cooperator: migration to 16.0 #86
Conversation
Signed-off-by: Carmen Bianca Bakker <[email protected]>
- remove coop_candidates from cooperators kanban view - keep only coop_candidates in coop_candidates view - remove legal reprentative in both
Signed-off-by: Carmen Bianca Bakker <[email protected]>
Signed-off-by: Carmen Bianca Bakker <[email protected]>
Signed-off-by: Carmen Bianca Bakker <[email protected]>
- refactor the form using templates - simplify the design: - replace tables by bootstrap columns - use columns for the overall form rather than for individual fields - use HTML5 widgets for the Part Numbers - use adequate bootstrap classes - use HTML5 date widget
fix incorrect call to ._get_share_transfer_mail_template() that was forgotten during the recent correction of "transfert" to "transfer".
* make subscription.request.name a stored computed field to allow to search by any part of the full name. * fix subscription request views: use only firstname and lastname.
Previously, if either firstname or lastname was (unexpectedly) empty, an error would be thrown. This commit uses approximately the same code approach as partner-contact/partner_firstname to compute the full name. Signed-off-by: Carmen Bianca Bakker <[email protected]>
Signed-off-by: Carmen Bianca Bakker <[email protected]>
fix error that happens when creating a company contact with a sub-contact.
fix creation of subscription request from partner by adding missing fields: firstname, lastname and lang.
Don't use value from self when looping over a list Signed-off-by: Carmen Bianca Bakker <[email protected]>
@polchampion: my answers below:
this is because the company has no subscription journal. it is created automatically when a chart of account is configured on the company (settings > invoicing > fiscale localization > package).
this has moved: it is now available from the cooperator register (registers > cooperator register).
this is because there are no configured share products available for companies. i added an error message in this case. |
@polchampion: my answers below:
this is because the company has no subscription journal. it is created automatically when a chart of account is configured on the company (settings > invoicing > fiscal localization > package).
this has moved: it is now available from the cooperator register (registers > cooperator register).
this is because there are no configured share products available for this type of partner (a company in this case). there should be at least one product marked as “is share?” and “can be subscribed by companies?”. i added an error message in this case. (coopiteasy#4) |
i’ve split changes into separate commits for clarity. this branch is almost ready (still waiting for reviews or fixes on coopiteasy#3 and coopiteasy#4). the module version is currently set at version 16.0.0.1.0 instead of 16.0.1.0.0 to indicate that it is still being developed. before merging, the migration folder should be renamed to 16.0.1.0.0 (although it would still work with its current name). this branch should be merged as a major change. this will update the version to 16.0.1.0.0. |
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.
Some commentary. This looks like a huge undertaking otherwise. Well done @huguesdk !
<t t-out="object.subscription_request_ids[0].firstname" />, | ||
<t | ||
t-if="object.is_company" | ||
t-out="object.child_ids.filtered(lambda c: c.representative).firstname" | ||
/> | ||
<t t-else="" t-out="object.firstname" />, |
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.
This entire if-else section seems like it should be its own computed convenience field. Especially because it's repeated in this file.
operation_type = fields.Selection( | ||
[ | ||
("subscription", "Subscription"), | ||
("transfer", "Transfer"), | ||
("sell_back", "Sell Back"), | ||
("convert", "Conversion"), |
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.
Does the removal of this selection option require a migration script?
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.
i don’t think so, because it has never been implemented.
# fixme: this field should be removed. it is only used for transfer | ||
# operations just to hold the values to create a new partner from. using a | ||
# subscription request for this causes several problems: it appears in the | ||
# list of subscription requests while it is not a real one and it sends an | ||
# email message to the receiver when it is created. instead, a partner | ||
# should be created. the problem with using a partner, though, (besides | ||
# creating a partner that might never be used if the the operation is | ||
# never executed) is that it cannot hold the extra values coming from the | ||
# subscription request and not stored on the partner or on the | ||
# cooperative.membership, like the iban (is it the only one?). should we | ||
# use a separate model that would be used here and in subscription.request | ||
# (possibly as a mixin)? | ||
subscription_request = fields.One2many( |
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.
imo subscription.request is an overused object throughout the cooperator codebase. It should just be an object that contains enough data to create a partner, an invoice, and share.lines, and is subsequently never referenced again. But it's used a lot in heaps of places.
I'm not sure what a good solution here is, though.
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.
i totally agree with you.
<t | ||
t-if="object.is_company" | ||
t-out="object.child_ids.filtered(lambda c: c.representative).firstname" | ||
/> | ||
<t t-if="object.is_company" t-out="object.get_representative().firstname" /> | ||
<t t-else="" t-out="object.firstname" />, |
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.
You almost read my mind! I still think this can be simplified into a single convenience field (or method) to get rid of the if-else :)
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.
this is specific to mail templates related to the res.partner
model. (for subscription requests, for instance, the firstname
field can be used directly.) do you have a suggestion? what kind of field would you add? main_person_firstname
? physical_person_firstname
? or would you change the way the firstname
field works for companies?
if not record.subscription_request.is_operation: | ||
raise ValidationError( | ||
_( | ||
"The is_operation field of the subscription " | ||
"request must be true" | ||
) | ||
) | ||
if record.subscription_request.source != "operation": | ||
raise ValidationError( | ||
_( | ||
"The source field of the subscription request " | ||
"must be set to operation" | ||
) | ||
) |
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.
Is this not testing the same thing twice? Is the second test really necessary?
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.
good point. i’ve found that the fields is_operation
, source
(and state
for transfer operations) were related to operations, and instead of changing the way subscription requests work, i added this here to make sure the ones used by operations are uniform. i think it would be a good idea to rethink these fields at some point.
@api.constrains("email", "company_email") | ||
def _check_company_and_representative_email_different(self): | ||
for record in self: | ||
if record.email == record.company_email: | ||
raise ValidationError(_("Email and Company Email must be different.")) | ||
|
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.
I'm not certain I like this constraint. I'm sure it solves a bug, but this seems like an incredibly silly restriction from the user's POV.
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.
agreed, but that limitation exists already. this only makes the error message understandable. (see the internal task)
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.
thanks for your re-review, @carmenbianca! here are my answers.
operation_type = fields.Selection( | ||
[ | ||
("subscription", "Subscription"), | ||
("transfer", "Transfer"), | ||
("sell_back", "Sell Back"), | ||
("convert", "Conversion"), |
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.
i don’t think so, because it has never been implemented.
# fixme: this field should be removed. it is only used for transfer | ||
# operations just to hold the values to create a new partner from. using a | ||
# subscription request for this causes several problems: it appears in the | ||
# list of subscription requests while it is not a real one and it sends an | ||
# email message to the receiver when it is created. instead, a partner | ||
# should be created. the problem with using a partner, though, (besides | ||
# creating a partner that might never be used if the the operation is | ||
# never executed) is that it cannot hold the extra values coming from the | ||
# subscription request and not stored on the partner or on the | ||
# cooperative.membership, like the iban (is it the only one?). should we | ||
# use a separate model that would be used here and in subscription.request | ||
# (possibly as a mixin)? | ||
subscription_request = fields.One2many( |
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.
i totally agree with you.
if not record.subscription_request.is_operation: | ||
raise ValidationError( | ||
_( | ||
"The is_operation field of the subscription " | ||
"request must be true" | ||
) | ||
) | ||
if record.subscription_request.source != "operation": | ||
raise ValidationError( | ||
_( | ||
"The source field of the subscription request " | ||
"must be set to operation" | ||
) | ||
) |
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.
good point. i’ve found that the fields is_operation
, source
(and state
for transfer operations) were related to operations, and instead of changing the way subscription requests work, i added this here to make sure the ones used by operations are uniform. i think it would be a good idea to rethink these fields at some point.
@api.constrains("email", "company_email") | ||
def _check_company_and_representative_email_different(self): | ||
for record in self: | ||
if record.email == record.company_email: | ||
raise ValidationError(_("Email and Company Email must be different.")) | ||
|
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.
agreed, but that limitation exists already. this only makes the error message understandable. (see the internal task)
<t | ||
t-if="object.is_company" | ||
t-out="object.child_ids.filtered(lambda c: c.representative).firstname" | ||
/> | ||
<t t-if="object.is_company" t-out="object.get_representative().firstname" /> | ||
<t t-else="" t-out="object.firstname" />, |
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.
this is specific to mail templates related to the res.partner
model. (for subscription requests, for instance, the firstname
field can be used directly.) do you have a suggestion? what kind of field would you add? main_person_firstname
? physical_person_firstname
? or would you change the way the firstname
field works for companies?
Functional test 4 - tested 27/11/2023 on 16-test-cooperator Setup
Basic flow
Features:
Added features during test
Configurations:
|
@huguesdk There are two share product for My Company available for companies, plus this partner is already cooperator with one of the type of shares. all other tests are ok ! |
@huguesdk and one minor thing : the "subscription register" should be renamed "operation register", although that should be checked and followed with Cath, not here |
sorry, i forgot one condition: the |
98286eb
to
772320b
Compare
@huguesdk ok, I updated yesterday's test report, all good.
|
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
Signed-off-by: Carmen Bianca BAKKER <[email protected]>
display an error message instead of failing when trying to create a subscription request from a partner and no default share product is found.
3b56711
to
93ad7f6
Compare
* update roadmap (contents and format). * remove obsolete install fragment. * fix link format in usage fragment.
/ocabot merge major |
Hey, thanks for contributing! Proceeding to merge this for you. |
Congratulations, your PR was merged at da48e0b. Thanks a lot for contributing to OCA. ❤️ |
No description provided.