-
-
Notifications
You must be signed in to change notification settings - Fork 206
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
[ADD][9.0]RMA (WIP) #110
[ADD][9.0]RMA (WIP) #110
Conversation
a72d4f3
to
675a4c0
Compare
8ce1294
to
7f9ab55
Compare
Dude I am really sorry. But this is a copy of our work in Vauxoo/rma (may be with little changes). I have several questions:
We had invested on this work a HUGE amount of work and we are opensourcing everything, but IMHO this is not the way to propose a PR or a Refactor... AFAICR we are all in the same team did not we? cc @jbeficent |
@nhomar Obviously we have to respect the authorship. We'll add that ASAP! We have been struggling with the current proposed approach in v8, and wanted to brainstorm with new ideas. We researched what has been done by your team, also we looked into Oracle, SAP, and Odoo v9 and v10, to try to identify new ideas to improve the process. We did not see https://github.com/Vauxoo/rma/pull/167. And we are going to review it now. We are happy to redirect the PR to Vauxoo/RMA and to work together on this! @yaniaular I understand that you are under active development. We would be happy to discuss together. |
@nhomar Of course we will respect the authorship, that was not intended, apologizes for the misunderstanding. |
You should move it to OCA, not to Vauxoo fork I think. |
Hi, First of all thank you for refactoring this module, it is an very old one that we took back from a version 5.0 and never had the budget to clean it up ! I support this move, and like very much the new way of handling the supplier and customer RMA. I've tested it and it works well overall. I have a few remarks:
That was helpful, in our case, we used this to record the picking in the system, so our customer can then print the shipping label properly, send it back to the customer in order for him to ship back to the proper place (without costs in our case). Now I didn't figured out how to do this anymore.
Apart from those points, I'm happy to support this refactoring ! Thanks again for the work, 👍 Joël |
d70cd75
to
684d09e
Compare
5bc2c24
to
919ccc5
Compare
919ccc5
to
99cfba9
Compare
I have a few points to review:
|
Hello @rubencabrera,
|
9a543e6
to
1c5ba52
Compare
Merge changes after pulling from 9.0 PR OCA#110
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.
Undefined variables in _default_dest/src_location_id methods
def _default_src_location_id(self): | ||
if self.type == 'customer': | ||
if self.rma_id.partner_id.property_stock_customer: | ||
return lines.rma_id.partner_id.property_stock_customer.id |
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.
Isn't lines an undefined variable here and in line 440?
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.
Yes, this method is not used indeed. I'll remove it. Thank you! @rubencabrera
@api.onchange('product_id') | ||
def _onchange_product_id(self): | ||
if not self.invoice_id: | ||
return |
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.
Hi @aheficent , is this by design? If RMA unrelated to invoice then onchange will not fill name field. Meanwhile field name is readonly thus data can not be saved.
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.
Hi @andhit-r Creating manual RMA lines was not possible at that time, but this design is going to be modified
from openerp.exceptions import UserError | ||
from dateutil.relativedelta import relativedelta | ||
from openerp.tools import DEFAULT_SERVER_DATE_FORMAT | ||
import math |
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.
unused import
from openerp.tools import DEFAULT_SERVER_DATE_FORMAT | ||
import math | ||
from datetime import datetime | ||
import calendar |
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.
unused import
default=lambda self: self.env.user) | ||
company_id = fields.Many2one('res.company', string='Company', | ||
default=lambda self: self.env.user.company_id) | ||
rma_line_ids = fields.One2many('rma.order.line', 'rma_id', |
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.
You should make it readonly unless it's draft
if self.env.context.get('supplier'): | ||
vals['name'] = self.env['ir.sequence'].get('rma.order.supplier') | ||
else: | ||
vals['name'] = self.env['ir.sequence'].get('rma.order') |
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.
IMO, the best way to assign sequence are according these flow:
- Give a default value for name (e.g. /)
- Remove readonly from field name, so user can override the sequence when they see fit
- If no change needed (field name value == /) then assign sequence when data created
Just an opinion, no need to change if you don't see fit.
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 your advice. It could be a good option.
self.move_count = len(list(set(move_list))) | ||
|
||
@api.model | ||
def _default_dest_location_id(self): |
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.
This method also unused
rma_id = fields.Many2one('rma.order', string='RMA', | ||
ondelete='cascade') | ||
uom_id = fields.Many2one('product.uom', string='Unit of Measure') | ||
product_id = fields.Many2one('product.product', string='Product', |
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.
Readonly unless its draft
Thank you for you review @andhit-r I hope to include some improvements soon |
Guys any idea of pushing this PR forward? |
Hi guys. Now I working in migrate this code to V 8.0, fix some errors and add new funcionality. I add module: rma_serial_number, this module allow select product and serial number, with this data the code can search the related invoice, (for example a big company have more mobile phone and a few moths since the company purchase this mobile have and problem with a unique serial number, but how this company have more invoices and pruchase order can't search the related invoice, for thah is this new funcionality). The second funcionality is when the company have and rma from a invoice haven't in the system becouse it is from the old system. For thah we have create a new object rma_external_invoice_line with the invoice number, date, client/supplier and serial number. can be this changes in this url: https://github.com/praxya/rma/tree/8.0-rma-serial-number regards |
Hi @berpweb We are still working on this. Regards. |
Can you rebase the PR to solve the conflict? |
@gurneyalex this is our current working branch https://github.com/Eficent/rma/tree/9.0-rma-v2 As we have deviated significantly from 8.0 we may not finally propose to OCA/rma to respect the current desig, used already by others. |
We have decided to continue our diverging approach in https://github.com/Eficent/stock-rma |
RMA
You use this module to manage RMA (Return Merchandise Authorization) in Odoo
The RMA can be send from the company to the customer or from the supplier to
the company
The RMA user creates the RMA and assign a responsible by selecting the
affected invoice. The available operations are refund, repair and deliver or
replace.
The assigned person checks if the conditions for the RMA are correct and
approve the RMA
The assigned person have to go to the RMA lines from the RMA and operate. THe
available options are creating receipts, delivery orders and refunds.
Once the assigned person considers that the RMA is settled he/she can
set the RMA to Done.
Configuration
To configure this module, you need to:
the option "Display 3 fields on rma: partner, invoice address, delivery
address" if needed
Warehouses and add a default RMA location and RMA picking type for customers
and suppliers RMA picking type. It's very important to select the type of
operation supplier if we are moving in the company and customer if we are
moving out of the company.
Usage
RMA are accessible though Inventory menu. There's four menus, divided by type
. Users can access to the list of RMA or RMA lines.
Create an RMA:
Order, Create Refund"
Issues
You can also comment about this module in this issue: #111
OBS: Additional modules will allow to create RMA from SO or PO