Tryton Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(585)

Issue 31881002: purchase_request_quotation: new module

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 2 weeks ago by mrichez
Modified:
3 days, 8 hours ago
Reviewers:
pokoli, ced, reviewbot
Visibility:
Public.

Description

issue6175 require:https://tryton-rietveld.appspot.com/30951002

Patch Set 1 #

Total comments: 12

Patch Set 2 : Applying last remarks #

Patch Set 3 : Applying last remarks #

Total comments: 84
Unified diffs Side-by-side diffs Delta from patch set Stats (+2141 lines, -0 lines) Patch
A .drone.yml View 1 chunk +9 lines, -0 lines 0 comments Download
A COPYRIGHT View 1 chunk +16 lines, -0 lines 1 comment Download
A INSTALL View 1 chunk +31 lines, -0 lines 1 comment Download
A LICENSE View 1 chunk +674 lines, -0 lines 0 comments Download
A MANIFEST.in View 1 chunk +13 lines, -0 lines 0 comments Download
A README View 1 chunk +20 lines, -0 lines 0 comments Download
A __init__.py View 1 chunk +24 lines, -0 lines 0 comments Download
A doc/index.rst View 1 1 chunk +31 lines, -0 lines 7 comments Download
A purchase.odt View Binary file 0 comments Download
A purchase.py View 1 2 1 chunk +509 lines, -0 lines 49 comments Download
A purchase.xml View 1 2 1 chunk +296 lines, -0 lines 14 comments Download
A setup.py View 1 chunk +119 lines, -0 lines 4 comments Download
A tests/__init__.py View 1 chunk +6 lines, -0 lines 1 comment Download
A tests/scenario_purchase_request_quotation.rst View 1 chunk +235 lines, -0 lines 3 comments Download
A tests/test_purchase_request_quotation.py View 1 chunk +28 lines, -0 lines 0 comments Download
A tox.ini View 1 chunk +18 lines, -0 lines 0 comments Download
A tryton.cfg View 1 chunk +12 lines, -0 lines 1 comment Download
A view/configuration_form.xml View 1 chunk +10 lines, -0 lines 0 comments Download
A view/purchase_request_create_quotation_ask_suppliers_form.xml View 1 chunk +9 lines, -0 lines 0 comments Download
A view/purchase_request_form.xml View 1 chunk +10 lines, -0 lines 2 comments Download
A view/purchase_request_quotation_group_form.xml View 1 chunk +33 lines, -0 lines 1 comment Download
A view/purchase_request_quotation_group_tree.xml View 1 chunk +8 lines, -0 lines 0 comments Download
A view/purchase_request_quotation_list.xml View 1 chunk +14 lines, -0 lines 0 comments Download
A view/purchase_request_quotation_list_sequence.xml View 1 chunk +16 lines, -0 lines 0 comments Download

Messages

Total messages: 11
mrichez
1 month, 2 weeks ago (2017-01-11 08:28:25 UTC) #1
reviewbot
patch is not applicable URL: https://codereview.tryton.org/31881002
1 month, 2 weeks ago (2017-01-11 08:44:02 UTC) #2
pokoli
https://tryton-rietveld.appspot.com/31881002/diff/1/.travis.yml File .travis.yml (right): https://tryton-rietveld.appspot.com/31881002/diff/1/.travis.yml#newcode1 .travis.yml:1: language: python This file is not required as we ...
1 month, 2 weeks ago (2017-01-13 12:59:47 UTC) #3
mrichez
https://tryton-rietveld.appspot.com/31881002/diff/1/.travis.yml File .travis.yml (right): https://tryton-rietveld.appspot.com/31881002/diff/1/.travis.yml#newcode1 .travis.yml:1: language: python On 2017/01/13 12:59:47, pokoli wrote: > This ...
1 month, 1 week ago (2017-01-16 11:00:49 UTC) #4
mrichez
Applying last remarks
1 month, 1 week ago (2017-01-16 11:01:17 UTC) #5
reviewbot
patch is not applicable URL: https://codereview.tryton.org/31881002
1 month, 1 week ago (2017-01-16 11:10:46 UTC) #6
pokoli
https://tryton-rietveld.appspot.com/31881002/diff/1/purchase.py File purchase.py (right): https://tryton-rietveld.appspot.com/31881002/diff/1/purchase.py#newcode322 purchase.py:322: self.raise_user_warning('warning', 'previous_quotation', { On 2017/01/16 11:00:49, mrichez wrote: > ...
1 month, 1 week ago (2017-01-16 11:37:19 UTC) #7
mrichez
https://tryton-rietveld.appspot.com/31881002/diff/1/purchase.py File purchase.py (right): https://tryton-rietveld.appspot.com/31881002/diff/1/purchase.py#newcode322 purchase.py:322: self.raise_user_warning('warning', 'previous_quotation', { On 2017/01/16 11:37:19, pokoli wrote: > ...
1 month, 1 week ago (2017-01-16 12:55:35 UTC) #8
mrichez
Applying last remarks
1 month, 1 week ago (2017-01-16 15:50:38 UTC) #9
reviewbot
patch is not applicable URL: https://codereview.tryton.org/31881002
1 month, 1 week ago (2017-01-16 16:13:48 UTC) #10
ced
1 month ago (2017-01-24 15:52:31 UTC) #11
https://tryton-rietveld.appspot.com/31881002/diff/40001/COPYRIGHT
File COPYRIGHT (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/COPYRIGHT#newcode3
COPYRIGHT:3: Copyright (C) 2016 SALUC SA
update to 2017

https://tryton-rietveld.appspot.com/31881002/diff/40001/INSTALL
File INSTALL (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/INSTALL#newcode10
INSTALL:10: * trytond_purchase_requisition (http://www.tryton.org/)
Not purchase_request?

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst
File doc/index.rst (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst#newcode5
doc/index.rst:5: to different suppliers through a wizard that will generate
quotations
"…to different suppliers through a selection process."

I think it is implementation detail to talk about wizard.

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst#newcode8
doc/index.rst:8: answer, user will encode supplier's data (price, unit,
currency, description).
Still too much implementation details.
This process could be customized etc.
So I think it is better to stick with:

Each request will collect quotation information from the supplier.
The selection of the quotation is done by taking the first ordered of the
answered requests.

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst#newcode13
doc/index.rst:13: Quotation Group
Probably better to name it "Quotation"

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst#newcode16
doc/index.rst:16: - Supplier: The supplier.
Maybe we need an address to sent the quotation?

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst#newcode17
doc/index.rst:17: - Quotations:
Name it "Lines"

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst#newcode27
doc/index.rst:27: - Amount: The amount of the current line (Unit Price
multiplied by Quantity).
I do  not think it is good to explain the computation, it could be customized.

https://tryton-rietveld.appspot.com/31881002/diff/40001/doc/index.rst#newcode31
doc/index.rst:31: - Company: The company which issue the request quotation.
could be defined just after the Supplier

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py
File purchase.py (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode23
purchase.py:23: STATES = [
Try to avoid such global definition. They are not useful after all.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode28
purchase.py:28: ('cancel', 'Canceled'),
canceled also for internal

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode46
purchase.py:46: class Group(Workflow, ModelSQL, ModelView):
Just Quotation

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode58
purchase.py:58: readonly=True, select=True)
I do not think it is needed to have a select here.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode59
purchase.py:59: request_quotations =
fields.One2Many('purchase.request.quotation', 'group',
Would name it: lines

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode59
purchase.py:59: request_quotations =
fields.One2Many('purchase.request.quotation', 'group',
Missing domain on company, supplier.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode71
purchase.py:71: ('sent', 'answered'),
I'm wondering if we should not allow to go from sent to draft in case there was
a mistake.
In this case we could just increase a version number?

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode95
purchase.py:95: @staticmethod
Do not use staticmethod for default value.
staticmethod are for constructors.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode112
purchase.py:112: if values.get('number') is None:
Should not the number be filled when sending?

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode159
purchase.py:159: class PurchaseRequestQuotation(
QuotationLine

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode160
purchase.py:160: sequence_ordered(), ModelSQL, ModelView):
I think it fill in one line.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode172
purchase.py:172: supply_date = fields.Date('Expected Supply Date')
The label should the same as the field name.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode210
purchase.py:210: ondelete='RESTRICT',
I think it should be CASCADE.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode211
purchase.py:211: readonly=True,
Why readonly?

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode213
purchase.py:213: ('company', '=', Eval('company', -1)),
Missing supplier

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode217
purchase.py:217: fields.Selection(STATES, 'Request for Quotation Group State'),
Better to use a function which takes the states from the group.
See https://bugs.tryton.org/issue6206

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode219
purchase.py:219: 
Try to keep the same order between field definition and there on_change/default
methods.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode223
purchase.py:223: return self.group.state
There is no None in the STATES.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode227
purchase.py:227: if (self.unit_price is None) or (self.quantity is None):
parenthesis are not needed.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode231
purchase.py:231: amount = self.currency.round(amount)
Otherwise it should be rounded to the default number of digits of the field.
But maybe we could be a default of None.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode260
purchase.py:260: def search_group_state(cls, name, clause):
should be defined close to the getter

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode279
purchase.py:279: requests = fields.Many2Many('purchase.request', None, None,
'Requests',
What is the point to show the requests as they are those that were selected.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode287
purchase.py:287: __name__ = 'purchase.request.create_quotation'
I think it is better named:

purchase.request.quotation.create

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode292
purchase.py:292: 'purchase_request_quotation.' +
No need of '+'

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode295
purchase.py:295: Button('Process', 'process', 'tryton-go-next', default=True),
I think it should be tryton-ok as it is the final step.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode297
purchase.py:297: process = StateTransition()
As it is mainly about creation, it should be named create_

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode319
purchase.py:319: self.raise_user_warning(str(request), 'previous_quotation', {
warning should not be raised on default method but on transition.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode330
purchase.py:330: def transition_start(self):
should be defined before the default_ask_suppliers

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode336
purchase.py:336: ('id', 'in', request_ids),
As the size of request_ids could be very large, I think it is better to filter
in Python.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode341
purchase.py:341: return 'end'
I think the user need to have a feedback in this case.
Maybe an error message.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode349
purchase.py:349: for request in getattr(self.ask_suppliers, 'requests', []):
You should not use getattr if it is supposed to be filled.
It is better to fail then silently recover from unexpected.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode350
purchase.py:350: new_quotations.append({
you should create an instance instead of dict, it is easier to manipulate.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode359
purchase.py:359: })
This creation should be customable, the best way is to have a method that return
new instance using the request as parameter.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode361
purchase.py:361: for supplier in getattr(self.ask_suppliers, 'suppliers', []):
idem about silent failure

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode365
purchase.py:365: if suppliers:
suppliers is required so no need to check it.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode370
purchase.py:370: for (nq, s) in product(new_quotations, suppliers):
The product could be done a the list of request and suppliers.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode382
purchase.py:382: key=self._group_purchase_request_quotation_key)
Instead of creating and after grouping. Is it not better to create already
grouped?
you could have a loop over supplier and inside loop over the request to create
one quotation with all the lines.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode412
purchase.py:412: active_quotations = fields.Function(fields.One2Many(
Better to move the prefix as suffix: quotations_active
This creates a sort of namespace.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode420
purchase.py:420: quotations = [q for q in self.active_quotations]
What is the point of active_quotations?
Is it not better to have quotations_answered?

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode423
purchase.py:423: quotations = sorted(quotations, key=keyfunc)
I think it is better to just define an order on the quotations field.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode439
purchase.py:439: state = 'quotation'
Should not we have a state which says if there is at least one answered
quotations?
And maybe a deadline on the quotation request.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode446
purchase.py:446: cls.write(requests, {
could be written:

if value:
   cls.write(…)

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode453
purchase.py:453: if quotation.group_state in ('draft', 'sent', 'answered'):
use set for in test, more logical:

… in {…}

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode475
purchase.py:475: def transition_init(self):
Why not override the transition_start ?
Changing a the global start_state is dangerous because it can break modularity
if other modules do the same.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode480
purchase.py:480: reqs = Request.search([
better to use full name: requests

it is not like you do not have place.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode481
purchase.py:481: ('id', 'in', request_ids),
idem about large list.
It is better to loop over instances.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode492
purchase.py:492: })
better to use the pattern:

for req in reqs:
  if …:
     req.party = …
Request.save(reqs)

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode499
purchase.py:499: key += (('unit_price', request.best_quotation.unit_price),)
I'm wondering if it makes sense to group per unit_price.
I think we can take the best price of all best_quotation in
compute_purchase_line.
It should be quite rare to have two quotation from the same supplier at the same
time with different price.
And if it happens, I think we can use the lowest price as we are buying even
more than this quotation.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.py#newcode508
purchase.py:508: key += (('currency', request.best_quotation.currency),)
You should override the currency property of request instead.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml
File purchase.xml (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode7
purchase.xml:7: id="purchase_request_quotation_group_view_tree">
it is a list not a tree.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode80
purchase.xml:80: </record>
What is the point of this access?

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode85
purchase.xml:85: </record>
idem

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode101
purchase.xml:101: </record>
What is the point of this access?

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode129
purchase.xml:129: </record>
Define sequence in a noupdate data block.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode161
purchase.xml:161: <record model="ir.rule.group"
id="rule_group_request_quotation_group">
Please define rule in the same section as the view of the same model.
Otherwise it is harder to check if every models have correct rules.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode165
purchase.xml:165: </record>
I do not understand those rules.
We just need company rules.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode203
purchase.xml:203: <field name="group"
ref="purchase_request.group_purchase_request"/>
I'm wondering if we should not have a specific group.
And the other purchase groups will have only read access.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode206
purchase.xml:206: <field name="perm_delete" eval="True"/>
Missing perm_create

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode215
purchase.xml:215: <field name="perm_delete" eval="True"/>
idem

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode233
purchase.xml:233: <field name="perm_delete" eval="True"/>
idem

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode242
purchase.xml:242: <field name="perm_delete" eval="True"/>
idem

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode264
purchase.xml:264: </record>
I think we will need a relate from requests to quotations.

https://tryton-rietveld.appspot.com/31881002/diff/40001/purchase.xml#newcode278
purchase.xml:278: <field
name="report">purchase_request_quotation/purchase.odt</field>
should be named quotation.odt or request_quotation.odt

https://tryton-rietveld.appspot.com/31881002/diff/40001/setup.py
File setup.py (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/setup.py#newcode15
setup.py:15: MODULE2PREFIX = {}
Wrong template for standard module.

https://tryton-rietveld.appspot.com/31881002/diff/40001/setup.py#newcode59
setup.py:59: description=('Tryton The purchase_request_quotation module of '
Do not duplicate the name of the module in the descrition.

https://tryton-rietveld.appspot.com/31881002/diff/40001/setup.py#newcode60
setup.py:60: 'the Tryton application platform.'),
Just use Tryton.

https://tryton-rietveld.appspot.com/31881002/diff/40001/setup.py#newcode63
setup.py:63: author_email='maxime.richez@saluc.com',
should be tracker email.

https://tryton-rietveld.appspot.com/31881002/diff/40001/tests/__init__.py
File tests/__init__.py (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/tests/__init__.py#new...
tests/__init__.py:4: from .test_purchase_request_quotation import suite
missing global import try. See the cookiecutter template.

https://tryton-rietveld.appspot.com/31881002/diff/40001/tests/scenario_purcha...
File tests/scenario_purchase_request_quotation.rst (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/tests/scenario_purcha...
tests/scenario_purchase_request_quotation.rst:11: >>> from
trytond.modules.company.tests.tools import (create_company,
Why not move create_company on the new line also, it will be more readable.

https://tryton-rietveld.appspot.com/31881002/diff/40001/tests/scenario_purcha...
tests/scenario_purchase_request_quotation.rst:163: >>> q =
Quotation.find([('group_state', '=', 'draft')])
Try to use explicit variables.

https://tryton-rietveld.appspot.com/31881002/diff/40001/tests/scenario_purcha...
tests/scenario_purchase_request_quotation.rst:204: ...         i = i + 1
avoid too complex code in scenario. If needed you can create a tools file
Or maybe we should implement first: https://bugs.tryton.org/issue6133

https://tryton-rietveld.appspot.com/31881002/diff/40001/tryton.cfg
File tryton.cfg (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/tryton.cfg#newcode2
tryton.cfg:2: version=4.1.0
4.3.0

https://tryton-rietveld.appspot.com/31881002/diff/40001/view/purchase_request...
File view/purchase_request_form.xml (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/view/purchase_request...
view/purchase_request_form.xml:6: <separator string="Quotations Info"
colspan="4" id="quotation"/>
I would just use "Quotations" as string.

https://tryton-rietveld.appspot.com/31881002/diff/40001/view/purchase_request...
view/purchase_request_form.xml:7: <field name="active_quotations" colspan="4"
widget="many2many" readonly="1"
80 cols

https://tryton-rietveld.appspot.com/31881002/diff/40001/view/purchase_request...
File view/purchase_request_quotation_group_form.xml (right):

https://tryton-rietveld.appspot.com/31881002/diff/40001/view/purchase_request...
view/purchase_request_quotation_group_form.xml:18: <group col="6" colspan="2"
id="buttons">
use col="-1"
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cd18842