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

[16.0][FIX] rma: group rma receptions #423

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

sbejaoui
Copy link
Contributor

fixes: #422

@OCA-git-bot
Copy link
Contributor

Hi @chienandalu, @pedrobaeza,
some modules you are maintaining are being modified, check this out!

@sbejaoui sbejaoui force-pushed the 16.0-rma_group_reception-sbj branch from c9815df to 3ac056c Compare August 29, 2024 17:38
@sbejaoui sbejaoui changed the title [FIX] rma: group rma receptions [16.0][FIX] rma: group rma receptions Sep 5, 2024
@sbejaoui sbejaoui force-pushed the 16.0-rma_group_reception-sbj branch from 3ac056c to e439f6a Compare September 5, 2024 09:22
@rousseldenis rousseldenis added this to the 16.0 milestone Oct 16, 2024
@@ -26,6 +26,9 @@ def _default_rma_mail_draft_template(self):
except ValueError:
return False

rma_reception_grouping = fields.Boolean(
string="Group RMA receptions", default=False
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a help string should be great

Copy link
Contributor

@rousseldenis rousseldenis left a comment

Choose a reason for hiding this comment

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

LGTM

@pedrobaeza
Copy link
Member

Seeing the original problem, this fix shouldn't be an option, but act always this way, don't you think?

@@ -732,6 +732,10 @@ def action_confirm(self):
self = self.filtered(lambda rma: rma.state == "draft")
if not self:
return
if self.env.company.rma_reception_grouping:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if self.env.company.rma_reception_grouping:
if not self.procurement_group_id:

Choose a reason for hiding this comment

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

Indeed, it should be the default behavior and then there is no need for an option. I don't see anything setting a value in procurement_group_id except create_receipt that you fixed here https://github.com/OCA/rma/pull/421/files#diff-3d6f82bd3aa96a98f1dd5cd6389cb926247243353dda39dc5bbf9924b53c98cbR715 and create_replace here https://github.com/OCA/rma/blob/16.0/rma/models/rma.py#L1212

So this fix should be obsolete as what Pedro is suggesting is already like that. Or there is something else setting a value here that needs to be overridden ?

Copy link
Member

Choose a reason for hiding this comment

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

You may set this value manually if you are interested to put a specific group, so let's keep this option open.

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.

RMA: group reception
5 participants