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.

Sign up for GitHub

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

Jump to bottom

[14.0][IMP] rma configured flow #334

Closed
wants to merge 5 commits into from
Closed

[14.0][IMP] rma configured flow #334

wants to merge 5 commits into from

Conversation

mt-software-de
Copy link
Contributor

@mt-software-de mt-software-de commented Dec 19, 2022 •
edited
Loading

This PR would add the possibility to configure the moment,
when we are able to create a reception, return and refund

It adds for each process a own field on the rma.operation:
create_receipt_timing
create_return_timing
create_refund_timing

By configure a field as on_confirm
the process will be created on action_confirm
Also we can now disable a process by set the field to no

With the addon rma_sale we can also configure the rma to no create refunds
but change the qty_delivered of the sale.order.line to create a refund over the sale.order

Depends on:
#350

@OCA-git-bot
Copy link
Contributor

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

chienandalu
chienandalu reviewed Dec 20, 2022
View reviewed changes
Copy link
Member

@chienandalu chienandalu left a comment

Choose a reason for hiding this comment

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

Thanks @mt-software-de . When you're ready, a rationale for these changes will be needed

@pedrobaeza
Copy link
Member

I think the rationale should go before the code changes, to validate it.

@mt-software-de
Copy link
Contributor Author

mt-software-de commented Dec 20, 2022 •
edited
Loading

Here is our scenario.
The Customer Support - knows at the beginning how the rma will take place
so they can select what pickings - refund should be created
So they are doing the confirm and everything is done.
For example: They don't have to wait for the return from the customer

There a different flows, as example

  • only returning
  • only refund
  • return and resend the goods (also at the same time, not only when return happend)
  • ...

Thats why the idea came up to make in configurable on the operation.
If its configured on the operation everything is created on action_confirm

@pedrobaeza @chienandalu

@mt-software-de mt-software-de force-pushed the 14.0-rma-configured-flow branch 3 times, most recently from e5d6f94 to 04299ac Compare December 20, 2022 11:00
@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 Apr 23, 2023
@mt-software-de mt-software-de mentioned this pull request May 22, 2023
Open
@mt-software-de mt-software-de force-pushed the 14.0-rma-configured-flow branch 7 times, most recently from da1c1ef to 505b7bd Compare May 23, 2023 12:10
@mt-software-de mt-software-de marked this pull request as ready for review May 23, 2023 13:08
@mt-software-de
Copy link
Contributor Author

mt-software-de commented May 23, 2023 •
edited
Loading

@chienandalu and @pedrobaeza now i'm done with my changes.
FYI: @jbaudoux

@mt-software-de mt-software-de force-pushed the 14.0-rma-configured-flow branch 3 times, most recently from 05d4850 to 62a4435 Compare May 25, 2023 09:37
@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 28, 2023
@mt-software-de mt-software-de force-pushed the 14.0-rma-configured-flow branch 3 times, most recently from f722d82 to 76d68f5 Compare May 31, 2023 17:55
@mt-software-de mt-software-de force-pushed the 14.0-rma-configured-flow branch 2 times, most recently from f6784b5 to d835eba Compare July 6, 2023 13:37
@mt-software-de mt-software-de mentioned this pull request Jul 6, 2023
Closed
@mt-software-de
[FIX] rma_sale_mrp: _check_rma_invoice_lines_qty checks now only line…
127c858 
…s with phantom_bom_product set~
@mt-software-de
[IMP] rma: use only procurement.group run to create stock transfers
5f1871d
@mt-software-de
[DON'T MERGE] test-requirements.txt
4c29466
@mt-software-de
[IMP] rma: Add possiblity to create receipts, returns and refunds on …
6dc6be8 
…action_confirm

By configure create_receipt_timing, create_return_timing, create_refund_timing
on a rma.operation we can the momemt when you are able to create the
different processes

As example: If you set create_return_timing to
 - "on_confirm" the return move will be created automatically on action_confirm
 - "after_receipt" the return can be created manually after the receipt
 - "no" a return can not be created
@mt-software-de
[FIX] rma_sale_mrp: Set bom_line_id on created moves to compute qty_d…
7b194cd 
…elivery correct
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 7, 2024

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 Apr 7, 2024
@jbaudoux jbaudoux added no stale Use this label to prevent the automated stale action from closing this PR/Issue. and removed stale PR/Issue without recent activity, it'll be soon closed automatically. labels Apr 8, 2024
jbaudoux
jbaudoux reviewed Jul 12, 2024
View reviewed changes
Copy link

@jbaudoux jbaudoux left a comment

Choose a reason for hiding this comment

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

some remarks

rma/models/rma.py
@@ -782,13 +846,14 @@ def action_refund(self):
refund = invoice_form.save()
line = refund.invoice_line_ids.filtered(lambda r: not r.rma_id)
line.rma_id = rma.id
rma.write(
_vals = deepcopy(vals)
Copy link

Choose a reason for hiding this comment

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

you could just build vals here without requiring deepcopy

rma/models/rma.py
for rma in rmas_to_return:
picking = rma.delivery_move_ids.picking_id.sorted("id", reverse=True)[0]
for rma in self:
# TODO: warum sind hier mehr als eins vorhanden
Copy link

Choose a reason for hiding this comment

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

comment can be dropped

rma/models/rma_operation.py
Comment on lines +11 to +12
TIMING_ON_CONFIRM_STR = "On confirm"
TIMING_AFTER_RECEIPT_STR = "After receipt"
Copy link

Choose a reason for hiding this comment

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

I would not create globals for the labels

@jbaudoux jbaudoux mentioned this pull request Jul 12, 2024
Open
@mt-software-de
Copy link
Contributor Author

some remarks

Thanks for the review but nothing will happen here in the near future. Due to the recent events in the repo rma, the contribution for this repo is stopped. Therefore I'm closing this PR.

@pedrobaeza
Copy link
Member

It's a pity that you take this decision, but I think the only one that is losing here is you. Anyway, part of the reasons for not accepting directly your PRs is changes that over-complicate the review and bring no value to the existing code. For example, in this PR, these changes are meaningless and even resting readability:

imagen

I'm also guilty here for not wanting to fight more battles - I tried in #350 (comment) and #350 (comment) and some other places, but each step means too much time, or the alternative to have unknown worse code to maintain, taking us to the option to rewrite from your foundations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chienandalu chienandalu chienandalu left review comments

@jbaudoux jbaudoux jbaudoux left review comments

@vuwnevska vuwnevska vuwnevska left review comments

Assignees
No one assigned
Labels
no stale Use this label to prevent the automated stale action from closing this PR/Issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mt-software-de @OCA-git-bot @pedrobaeza @chienandalu @jbaudoux @vuwnevska