[16.0][IMP] rma: create incoming shipment from rma group #508

chafique-delli · 2024-03-25T16:53:35Z

Everything that is created via the wizards at the level of an RMA line, we wish to do at the level of the RMA group, namely:

create a single receipt order to process several RMA Group lines
create a single delivery order to process several RMA Group lines
create a single supplier RMA to process several lines of the RMA Group
create a single refund to process several lines of the RMA Group
create a single sale order to process several lines of the RMA Group
I would like to have your opinion on this process.
If this does not pose a problem for you then I will continue my work to extend this to other wizards ('CREATE DELIVERY', 'CREATE SUPPLIER RMA', ....).
@CarlosVForgeFlow , @florian-dacosta , @AaronHForgeFlow

codecov-commenter · 2024-03-25T16:59:40Z

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 80.56%. Comparing base (3c0e9c4) to head (4e82915).

Additional details and impacted files

@@            Coverage Diff             @@
##             16.0     #508      +/-   ##
==========================================
- Coverage   80.58%   80.56%   -0.03%     
==========================================
  Files         128      128              
  Lines        4889     4898       +9     
  Branches      794      796       +2     
==========================================
+ Hits         3940     3946       +6     
- Misses        728      730       +2     
- Partials      221      222       +1

Files	Coverage Δ
rma/models/rma_order.py	`79.78% <100.00%> (+0.55%)`	⬆️
rma/wizards/rma_make_picking.py	`77.21% <50.00%> (-1.36%)`	⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c0e9c4...4e82915. Read the comment docs.

florian-dacosta · 2024-04-03T15:30:53Z

rma/wizards/rma_make_picking_view.xml

@@ -108,7 +108,7 @@
                    name="%(action_rma_picking_in)d"
                    string="Create Incoming Shipment"
                    class="oe_highlight"
-                    attrs="{'invisible':['|', '|', '|', ('qty_to_receive', '=', 0), ('qty_to_receive', '&lt;', 0), ('state', '!=', 'approved'), ('receipt_policy', '=', 'no')]}"
+                    attrs="{'invisible':['|', '|', ('qty_to_receive', '&lt;', 0), ('state', '!=', 'approved'), ('receipt_policy', '=', 'no')]}"
                    type="action"


Why this change ? Reading the code, it seems that the create incoming shipment button will be displayed twice if qty_to_receive = 0

@florian-dacosta ,
Reading the calculation method for the 'qty_to_receive' field, it seems to me that qty_to_receive=0.0 if receipt_policy not in ["ordered", "delivered"](so for receipt_policy='no').
This is why I told myself that putting in the domain ('qty_to_receive', '=', 0) and ('receipt_policy', '=', 'no') was a duplicate.

@api.depends(
"move_ids",
"move_ids.state",
"qty_received",
"receipt_policy",
"product_qty",
"type",
)
def _compute_qty_to_receive(self):
for rec in self:
rec.qty_to_receive = 0.0
if rec.receipt_policy == "ordered":
rec.qty_to_receive = rec.product_qty - rec.qty_received
elif rec.receipt_policy == "delivered":
rec.qty_to_receive = rec.qty_delivered - rec.qty_received

@chafique-delli But qty_to_receive may be 0 too for receipt_policy in ["ordered", "delivered"]. And in this case, we want to hide this button, as the other not highlighted will be displayed then.

For now with this change, if receipt_policy = "ordered" and qty_to_receive = 0, we have twice the same button (highlighted and not highlighted)

florian-dacosta · 2024-04-03T15:39:43Z

rma/wizards/rma_make_picking_view.xml

+                <button
+                    name="%(action_rma_picking_in)d"
+                    string="Create Incoming Shipment"
+                    attrs="{'invisible':['|', '|', ('qty_to_receive', '=', 0), ('qty_to_receive', '>', 0), ('state', '!=', 'approved')]}"


This button would be hided if qty_to_receive = 0 but displayed if qty_to_receive < 0, it does not seem logic to me.
If it is visible when qty_to_receive < 0, it should also be visible when qty_to_receive = 0.
Also, I believe you need to hide this button if no lines have a receipt policy set, like it is on rma line side.

I believe we have 2 options :

We behave in a different manner than on line side : we display only the receive button when we expected a reception (qty_to_receive > 0) and we don't display the button in the other case. Then, if the user want to force a reception, he should go on rma line where the button is displayed even if we don't expect him to generate a reception.
It may be a bit confusing for the user to have a different behavior but if seems better/safer to me since on group, we manage multiple lines...

Act like on rma line side exactly, then you have to add a field on group with the receipt policy, to hide the button if no policy is set.

@sebastienbeau, it would be nice to have your opinion on this

I think the best solution is the 1. User need a simple UI for processing the 99% of case. So only making visible the right button when there is something to do is the best option.
Then for the advanced case the user can go to the line and make some specific action.

Thanks for your work

I agree with @sebastienbeau in this case. Better to stick to the most common use case.

florian-dacosta · 2024-04-03T15:41:35Z

rma/wizards/rma_make_picking.py

+        if active_model == "rma.order":
+            rma = rma_obj.browse(active_ids)
+            lines = rma.rma_line_ids.filtered(lambda x: x.state == "approved")
+        else:


I don't think all approved lines should always be added when coming from a group.
I'd say, if the highlighted button is pressed, then we should only get lines having a qty_to_receive > 0
(maybe passing a context on the button for instance ?)
And if the not highlighted button is pressed, then all lines with a receipt policy != 0?

@florian-dacosta , it's done.

chafique-delli · 2024-04-19T14:46:28Z

@AaronHForgeFlow , @JordiBForgeFlow , Please, I would like to have your opinion on processing RMA lines from the RMA group.

AaronHForgeFlow · 2024-04-19T14:50:33Z

@chafique-delli I have not looked much into the code. But the idea of being able to generate a single receipts/deliveries/refunds for a RMA group seems like a good feature to me.

chafique-delli · 2024-04-19T15:08:29Z

@chafique-delli I have not looked much into the code. But the idea of being able to generate a single receipts/deliveries/refunds for a RMA group seems like a good feature to me.

Ok, if that suits you, I will continue in this direction. THANKS

chafique-delli · 2024-06-07T09:36:17Z

@sebastienbeau , @florian-dacosta , @AaronHForgeFlow , a review please.

AaronHForgeFlow

Tested so far:

create a single receipt order to process several RMA Group lines
create a single delivery order to process several RMA Group lines

It works good. Thanks. Do you plan to continue?

chafique-delli · 2024-07-04T18:01:12Z

Tested so far:

create a single receipt order to process several RMA Group lines

create a single delivery order to process several RMA Group lines

It works good. Thanks. Do you plan to continue?

I have completed the changes planned for the RMA Group.

AaronHForgeFlow · 2024-07-05T08:21:30Z

Thank you @chafique-delli I will review it soon

AaronHForgeFlow · 2024-07-08T15:49:58Z

@chafique-delli Thank you for the work, only a couple of things:

The refunds for the groups are created good, but for some reason the button in the rma group that takes the user to the created refund it is not working, it does nothing:

The sale order creation is correct, and linked correctly to the rma lines of the group. I think a small but nice addition here is to be able to access the generated sales order directly from the group, same as we do for the incoming shipments, deliveries or refunds. Do you think you can do it?

Aside from this. I think the only think pending is the discussion about the conditions on the displaying of the buttons.

chafique-delli · 2024-07-11T09:03:27Z

Aside from this. I think the only think pending is the discussion about the conditions on the displaying of the buttons.

@AaronHForgeFlow , thanks for the review.
For the 'Refunds' button, I'll see why it doesn't work.
For 'Sales' button, I will look to add this to the rma group.

florian-dacosta

@chafique-delli
I think we have a problem of consistency. There is a different behavior for each button and I find it very confusing.

Incoming shipment and outgoing shipment : Only visible if there is at least one line with a qty to receive/to_deliver
Take by default only rma lines with a qty_to_receive/deliver > 0

Create refund : always visible. Not highlighted if no quantity to refund on any line / highlighted if at least one line with a qty.
Then by default in the wizard, we have all lines, even those which would not have the refund in its operation (in case of different operation by line).

Create SO => Behave like create refund.

In my opinion, we should have the same behavior as for incoming/outgoing shipments. (1 button + take only lines which have a qty to refund / 1 button + only lines with a qty_to_sell > 0)

florian-dacosta · 2024-07-26T12:42:59Z

rma_account/wizards/rma_refund.py

-        lines = rma_line_obj.browse(rma_line_ids)
+        if active_model == "rma.order":
+            rma = rma_obj.browse(active_ids)
+            lines = rma.rma_line_ids


shouldn't we have the same logic as for incoming/outgoing shipment creation, take only the line with qty_to_refund > 0?

florian-dacosta · 2024-07-26T13:04:16Z

rma_account/wizards/rma_refund.py

+            lines = rma.rma_line_ids
+        else:
+            lines = rma_line_obj.browse(active_ids)
+        for line in lines:


I think this part does not make sense. We check all lines from rma group (or selected rma lines from ui via active_ids) are approved... But then, the refund is created on the base of the wizard item_ids, which may not be related to the checked lines...
Example : select 2 rma lines first one is not approved, second one is ok.
Click on action => create refund. Delete the first line because you changed your mind => click on invoice_refund. It will block because the first line is not approved... But this line won't even be in the refund since it has been deleted from the wizard.

I'd just check the rma lines doing something like lines = self.item_ids.line_id instead. It workfs for both case (rma lines/rma group). Does it make sense to you?

florian-dacosta · 2024-07-26T13:29:26Z

rma_sale/wizards/rma_make_sale_order.py

+                rma = rma_obj.browse(active_ids)
+                lines = rma.rma_line_ids
+            else:
+                lines = rma_line_obj.browse(active_ids)


I don't understand this whole method. Is this really used ? I think it could be removed fully...
It is a compute method from line_id field to link the item line with the rma_line, but on the loop, it will always take the same rma_line right ? (the first one in active_ids/rma order). So this method assign all item line to the same rma_line.

Then in default_get it is well set...I guess since it is set on default_get, this _compute_line_id is kind of useless, that is why it is working... I'd remove it totally...

florian-dacosta · 2024-07-26T13:29:50Z

rma_sale/wizards/rma_make_sale_order.py

-        lines = rma_line_obj.browse(rma_line_ids)
+        if active_model == "rma.order":
+            rma = rma_obj.browse(active_ids)
+            lines = rma.rma_line_ids


only those we qty_to_sell > 0 ?

florian-dacosta · 2024-07-26T13:32:39Z

rma/wizards/rma_make_picking_view.xml

@@ -134,4 +134,28 @@
        </field>
    </record>

+    <record id="view_rma_button_form" model="ir.ui.view">


I find it quite confusing to have an override of rma.order view in rma_make_picking_view.xml file.
I know there is other similar cases in the rma modules but do we really want to continue this way ? Should it not go to rma_order_view.xml ? @AaronHForgeFlow

@florian-dacosta, better to have file for model, whenever possible, in the next versions some cleanup will be done, hopefully.

chafique-delli · 2024-09-16T16:43:59Z

@chafique-delli Thank you for the work, only a couple of things:

The refunds for the groups are created good, but for some reason the button in the rma group that takes the user to the created refund it is not working, it does nothing:

The sale order creation is correct, and linked correctly to the rma lines of the group. I think a small but nice addition here is to be able to access the generated sales order directly from the group, same as we do for the incoming shipments, deliveries or refunds. Do you think you can do it?

Aside from this. I think the only think pending is the discussion about the conditions on the displaying of the buttons.

@AaronHForgeFlow , for the refund button, it's fixed and I added the smart button for the sales order.

florian-dacosta

All seems fine to me now, thanks

sebastienbeau

LGTM (code review)

florian-dacosta · 2024-11-08T09:04:59Z

@AaronHForgeFlow I believe this PR is ready, when you have time to finish the review!

(I'll port it to 17 when merged)

…om wizard views to rma_order_view.xml files

… made during the review

AaronHForgeFlow · 2024-11-11T12:52:53Z

Nice improvement. Functional tests look good so far. I will ask more people affected by this change to review this.

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch from 4e82915 to 0e68988 Compare March 28, 2024 16:12

florian-dacosta suggested changes Apr 3, 2024

View reviewed changes

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch 2 times, most recently from 35e7ac5 to 6fd5d2f Compare April 19, 2024 14:32

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch from a4456df to d657fa1 Compare April 22, 2024 16:13

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch from d657fa1 to 8c6cbcb Compare May 31, 2024 13:33

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch from 8c6cbcb to 8d59e55 Compare June 25, 2024 08:10

AaronHForgeFlow self-requested a review June 25, 2024 08:12

AaronHForgeFlow reviewed Jun 28, 2024

View reviewed changes

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch from 7eba3ea to e84ed45 Compare July 4, 2024 08:54

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch from 471941b to 4167a0a Compare July 5, 2024 15:07

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch 2 times, most recently from 23df512 to 58d68a5 Compare July 11, 2024 20:32

florian-dacosta reviewed Jul 26, 2024

View reviewed changes

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch 7 times, most recently from ab99e40 to 38d71bb Compare September 10, 2024 10:02

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch 6 times, most recently from 4195a38 to 5834842 Compare September 16, 2024 16:38

AaronHForgeFlow self-requested a review September 17, 2024 09:44

chafique-delli force-pushed the 16-create-incoming-shipment-in-rma-group branch 2 times, most recently from 3b54408 to 3c88cee Compare September 24, 2024 10:54

florian-dacosta approved these changes Oct 15, 2024

View reviewed changes

sebastienbeau approved these changes Nov 8, 2024

View reviewed changes

chafique-delli added 9 commits November 8, 2024 11:22

[16.0][IMP] rma: create incoming shipment from rma group

7ac3920

[IMP] create delivery from rma group

3523e5b

[IMP] create supplier rma from rma group

b5bac06

[IMP] create refund from rma group

e0ac4f1

[IMP] create sale order from rma group

1d3e984

[ADD] french translation for RMA module

c61ca33

[IMP] rma, rma_account, rma_sale: move override of rma.order views fr…

a6df22c

…om wizard views to rma_order_view.xml files

[FIX] rma, rma_account and rma_sale: following the comments that were…

1abc58e

… made during the review

[ADD] smart button for sales in rma group

60cdf67

florian-dacosta force-pushed the 16-create-incoming-shipment-in-rma-group branch 3 times, most recently from 75a4f5d to ac0b63b Compare November 8, 2024 11:41

[FIX] rma : fix label and translations for incoming shipments

699923e

florian-dacosta force-pushed the 16-create-incoming-shipment-in-rma-group branch from ac0b63b to 699923e Compare November 8, 2024 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16.0][IMP] rma: create incoming shipment from rma group #508

[16.0][IMP] rma: create incoming shipment from rma group #508

chafique-delli commented Mar 25, 2024 •

edited

Loading

codecov-commenter commented Mar 25, 2024

florian-dacosta Apr 3, 2024

chafique-delli Apr 16, 2024

florian-dacosta Apr 17, 2024

florian-dacosta Apr 3, 2024

sebastienbeau Apr 18, 2024

AaronHForgeFlow Jul 8, 2024

florian-dacosta Apr 3, 2024

chafique-delli Apr 16, 2024

chafique-delli commented Apr 19, 2024

AaronHForgeFlow commented Apr 19, 2024

chafique-delli commented Apr 19, 2024

chafique-delli commented Jun 7, 2024

AaronHForgeFlow left a comment

chafique-delli commented Jul 4, 2024

AaronHForgeFlow commented Jul 5, 2024

AaronHForgeFlow commented Jul 8, 2024

chafique-delli commented Jul 11, 2024

florian-dacosta left a comment

florian-dacosta Jul 26, 2024

florian-dacosta Jul 26, 2024

florian-dacosta Jul 26, 2024

florian-dacosta Jul 26, 2024

florian-dacosta Jul 26, 2024

AaronHForgeFlow Sep 10, 2024

chafique-delli commented Sep 16, 2024

florian-dacosta left a comment

sebastienbeau left a comment

florian-dacosta commented Nov 8, 2024

AaronHForgeFlow commented Nov 11, 2024

[16.0][IMP] rma: create incoming shipment from rma group #508

Are you sure you want to change the base?

[16.0][IMP] rma: create incoming shipment from rma group #508

Conversation

chafique-delli commented Mar 25, 2024 • edited Loading

codecov-commenter commented Mar 25, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chafique-delli commented Apr 19, 2024

AaronHForgeFlow commented Apr 19, 2024

chafique-delli commented Apr 19, 2024

chafique-delli commented Jun 7, 2024

AaronHForgeFlow left a comment

Choose a reason for hiding this comment

chafique-delli commented Jul 4, 2024

AaronHForgeFlow commented Jul 5, 2024

AaronHForgeFlow commented Jul 8, 2024

chafique-delli commented Jul 11, 2024

florian-dacosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chafique-delli commented Sep 16, 2024

florian-dacosta left a comment

Choose a reason for hiding this comment

sebastienbeau left a comment

Choose a reason for hiding this comment

florian-dacosta commented Nov 8, 2024

AaronHForgeFlow commented Nov 11, 2024

chafique-delli commented Mar 25, 2024 •

edited

Loading