-
-
Notifications
You must be signed in to change notification settings - Fork 798
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
[18.0][MIG] purchase_cancel_reason: Migration to 18.0 #2484
base: 18.0
Are you sure you want to change the base?
[18.0][MIG] purchase_cancel_reason: Migration to 18.0 #2484
Conversation
…_cancel_reason (OCA/sale-workflow) developed by CampToCamp. (OCA#438) * [ADD] Add the module purchase_cancel_reason. Copy/past from the module sale_cancel_reason (OCA/sale-workflow) developed by CampToCamp.
Currently translated at 40.9% (9 of 22 strings) Translation: purchase-workflow-14.0/purchase-workflow-14.0-purchase_cancel_reason Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-14-0/purchase-workflow-14-0-purchase_cancel_reason/pt_BR/
Currently translated at 36.3% (8 of 22 strings) Translation: purchase-workflow-14.0/purchase-workflow-14.0-purchase_cancel_reason Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-14-0/purchase-workflow-14-0-purchase_cancel_reason/it/
Currently translated at 36.3% (8 of 22 strings) Translation: purchase-workflow-14.0/purchase-workflow-14.0-purchase_cancel_reason Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-14-0/purchase-workflow-14-0-purchase_cancel_reason/it/
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate. Translation: purchase-workflow-17.0/purchase-workflow-17.0-purchase_cancel_reason Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-17-0/purchase-workflow-17-0-purchase_cancel_reason/
Currently translated at 100.0% (21 of 21 strings) Translation: purchase-workflow-17.0/purchase-workflow-17.0-purchase_cancel_reason Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-17-0/purchase-workflow-17-0-purchase_cancel_reason/it/
Currently translated at 100.0% (21 of 21 strings) Translation: purchase-workflow-17.0/purchase-workflow-17.0-purchase_cancel_reason Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-17-0/purchase-workflow-17-0-purchase_cancel_reason/sv/
Currently translated at 100.0% (21 of 21 strings) Translation: purchase-workflow-17.0/purchase-workflow-17.0-purchase_cancel_reason Translate-URL: https://translation.odoo-community.org/projects/purchase-workflow-17-0/purchase-workflow-17-0-purchase_cancel_reason/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.
LG, proposing small improvements but not blocking
from odoo.tests.common import TransactionCase | ||
|
||
|
||
class TestPurchaseCancelReason(TransactionCase): |
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.
We could inherit from BaseCommon
class to disable tracking on all records created in tests.
def setUp(self): | ||
super().setUp() |
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.
Small improvement, but as there is only one test this won't change anything here, so not blocking.
def setUp(self): | |
super().setUp() | |
@classmethod | |
def setUpClass(cls): | |
super().setUpClass() |
acd810b
to
5427288
Compare
@sebalix , Thank you for your insightful comment, which has helped improve the code. |
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! I did a functional test. The spacing looks off on a cancelled purchase order. It seems to say
Cancellation reason:Service no longer needed
A space would be nice, to make it say
Cancellation reason: Service no longer needed
Then again, I'm not fond of this unconventional way of displaying what is really a regular field. Would you consider just showing the field, under the condition that it has a value, for instance under Vendor Reference
?
/ocabot migration purchase_cancel_order |
5427288
to
62168f4
Compare
@StefanRijnhart , Thank you for your insightful comment, which has helped improve the code. |
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 the update, but you can just make this a simple <field name="cancel_reason_id" />
. No need for inline layout in this part of the form. That way, you can get rid of the layout tweak distinction between debug and non-debug mode (although it's well caught, and in other circumstances would be a sophisticated solution).
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 - Functional Review 👍
It has been verified that purchase orders can be canceled individually, with a cancellation reason correctly assigned within the order. However, when canceling multiple purchase orders simultaneously, the option to include a cancellation reason is not available.
Could we add functionality in this view to optionally include a reason when canceling? This would enhance the traceability and flexibility of the process.
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.
Ah yes, that's right! Odoo 18 supports action buttons defined in list views as well, and button_cancel
is added to two of those list views in the purchase module, e.g. https://github.com/odoo/odoo/blob/18.0/addons/purchase/views/purchase_views.xml#L596-L600. They would have to be replaced by this module's action_purchase_order_cancel
as well.
62168f4
to
5221b06
Compare
@StefanRijnhart , @sheragar , Thanks for the suggestion to improve traceability! It was a great idea, and it helped streamline the process. I’ve fixed the issue and pushed the changes. |
No description provided.