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

[14.0][IMP] purchase_sale_inter_company: the sale order is updated when the purchase order is modified #391

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

Conversation

chafique-delli
Copy link
Contributor

@chafique-delli chafique-delli commented Oct 14, 2022

Features added to the module:
If an inter-company purchase order is validated, its modification is prevented.
If you want to modify a purchase order, you have to cancel it first, then put it back in draft so that you can modify it and finally revalidate it.
When you revalidate the purchase order after modification, it canceled the sales order and recreates one.

@chafique-delli
Copy link
Contributor Author

@sebastienbeau

@sebastienbeau sebastienbeau force-pushed the 14.0-IMP-purchase_sale_inter_company branch from dea300d to f41bfd3 Compare November 28, 2022 12:47
@chafique-delli
Copy link
Contributor Author

up @sebastienbeau

@francesco-ooops
Copy link
Contributor

@chafique-delli can you rebase in order to trigger runbot rebild?

@chafique-delli chafique-delli force-pushed the 14.0-IMP-purchase_sale_inter_company branch from f41bfd3 to 96c0a54 Compare January 24, 2023 14:48
@chafique-delli
Copy link
Contributor Author

@chafique-delli can you rebase in order to trigger runbot rebild?

@francesco-ooops , it's done.

@elvise
Copy link

elvise commented May 23, 2023

@chafique-delli can you rebase ?

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Sep 24, 2023
@francesco-ooops
Copy link
Contributor

@chafique-delli did you implement this successfully in the end?

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 1, 2023
@chafique-delli chafique-delli force-pushed the 14.0-IMP-purchase_sale_inter_company branch from 96c0a54 to 3f3ebd4 Compare October 10, 2023 15:38
@sebastienbeau sebastienbeau force-pushed the 14.0-IMP-purchase_sale_inter_company branch from 3f3ebd4 to e8566bd Compare October 15, 2023 14:55
sebastienbeau
sebastienbeau approved these changes Oct 15, 2023
"cancel",
]:
force_number = self.intercompany_sale_id.name
self.intercompany_sale_id.with_context(force_delete=True).unlink()
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this.
If a sale order already have a picking and you cancel it. I pretty sure you can not delete the sale.

Maybe we should add a try except and if we can unlink it, we can keep the same number. If not we should use a new number as sale name should be uniq

@sebastienbeau sebastienbeau force-pushed the 14.0-IMP-purchase_sale_inter_company branch from e8566bd to 9b0a353 Compare October 15, 2023 16:58
@sebastienbeau sebastienbeau force-pushed the 14.0-IMP-purchase_sale_inter_company branch from 9b0a353 to ac1b9cc Compare December 7, 2023 23:36
Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

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

Some minor issues from code review.

Most importantly, from a functional POV: I agree with canceling the SO if the PO is canceled. I'm not so sure about having a new SO with the same name if the PO is then re-confirmed. It could be misleading if the PO has changed, but the user on the SO side thinks they're looking at the same one as before.

In general, also, I'm against deleting things (as opposed to canceling/archiving). Preserving the history of what happened is important. There can be attachments, comments, or edits on the SO that would suddenly be deleted as well. If there's no issue with making a new SO, I would make a new SO instead.

Comment on lines 197 to 222
if self.intercompany_sale_id:
if self.intercompany_sale_id.state not in ["draft", "sent", "cancel"]:
raise UserError(
_(
"You can not cancel your purchase order %s.\n"
"The sale order %s at your supplier %s that is linked "
"to your purchase order, is in the state %s.\n"
"In order to cancel your purchase order, you must first "
"ask your supplier to cancel his sale order."
)
% (
self.name,
self.intercompany_sale_id.name,
self.partner_id.name,
self.intercompany_sale_id.state,
)
)
self.intercompany_sale_id.sudo().action_cancel()
self.write({"partner_ref": False})
Copy link
Contributor

Choose a reason for hiding this comment

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

question: do you not need sudo to read these fields self.intercompany_sale.id.state, self.intercompany_sale_id.name ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I think we need the sudo here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kev-Roche , it's fixed.

Comment on lines 197 to 222
if self.intercompany_sale_id:
if self.intercompany_sale_id.state not in ["draft", "sent", "cancel"]:
raise UserError(
_(
"You can not cancel your purchase order %s.\n"
"The sale order %s at your supplier %s that is linked "
"to your purchase order, is in the state %s.\n"
"In order to cancel your purchase order, you must first "
"ask your supplier to cancel his sale order."
)
% (
self.name,
self.intercompany_sale_id.name,
self.partner_id.name,
self.intercompany_sale_id.state,
)
)
self.intercompany_sale_id.sudo().action_cancel()
self.write({"partner_ref": False})
Copy link
Contributor

Choose a reason for hiding this comment

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

question: why delete the partner_ref? The PO is still the same even if it was cancelled

</field>
<field name="order_line" position="attributes">
<attribute name="attrs" operation="python_dict" key="readonly">
['|', ('state', 'in', ('done', 'cancel')), ('intercompany_sale_id', '!=', False), ('state', '=', 'purchase')]
Copy link
Contributor

@aleuffre aleuffre Dec 11, 2023

Choose a reason for hiding this comment

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

issue: I'm not 100% sure about the order of operations, so, for readability if nothing else, I'd change this to:

Suggested change
['|', ('state', 'in', ('done', 'cancel')), ('intercompany_sale_id', '!=', False), ('state', '=', 'purchase')]
['|',
('state', 'in', ('done', 'cancel')),
'&nbsp;'
('intercompany_sale_id', '!=', False),
('state', '=', 'purchase')]

Translated: Always make it readonly during states done and cancel. If there is an intercompany_sale_id, make it readonly during purchase as well.

@legalsylvain legalsylvain added this to the 14.0 milestone Dec 17, 2023
@francesco-ooops
Copy link
Contributor

@chafique-delli do you plan on working further on this one?

@chafique-delli
Copy link
Contributor Author

do you plan on working further on this one

Yes, from March, I will have more time to think about the logic of canceling the PO as suggested by @aleuffre

@chafique-delli chafique-delli force-pushed the 14.0-IMP-purchase_sale_inter_company branch 3 times, most recently from e02e7ff to df1dbd8 Compare March 26, 2024 08:51
@chafique-delli chafique-delli force-pushed the 14.0-IMP-purchase_sale_inter_company branch from df1dbd8 to a3568be Compare March 26, 2024 12:30
@francesco-ooops
Copy link
Contributor

@chafique-delli let us know when ready for review :)

Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 28, 2024
@francesco-ooops
Copy link
Contributor

@chafique-delli do you think this will be completed eventually?

@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Sep 1, 2024
Copy link

github-actions bot commented Jan 5, 2025

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jan 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs fixing stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants