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

Issue 31881002: purchase_request_quotation: new module

Can't Edit
Can't Publish+Mail
Start Review
1 month, 2 weeks ago by mrichez
3 days, 8 hours ago
pokoli, ced, reviewbot


issue6175 require:

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 View 1 chunk +13 lines, -0 lines 0 comments Download
A README View 1 chunk +20 lines, -0 lines 0 comments Download
A 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 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 View 1 chunk +119 lines, -0 lines 4 comments Download
A tests/ 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/ 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


Total messages: 11
1 month, 2 weeks ago (2017-01-11 08:28:25 UTC) #1
patch is not applicable URL:
1 month, 2 weeks ago (2017-01-11 08:44:02 UTC) #2
pokoli File .travis.yml (right): .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 File .travis.yml (right): .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
Applying last remarks
1 month, 1 week ago (2017-01-16 11:01:17 UTC) #5
patch is not applicable URL:
1 month, 1 week ago (2017-01-16 11:10:46 UTC) #6
pokoli File (right): 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 File (right): 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
Applying last remarks
1 month, 1 week ago (2017-01-16 15:50:38 UTC) #9
patch is not applicable URL:
1 month, 1 week ago (2017-01-16 16:13:48 UTC) #10
1 month ago (2017-01-24 15:52:31 UTC) #11
File COPYRIGHT (right):
COPYRIGHT:3: Copyright (C) 2016 SALUC SA
update to 2017
File INSTALL (right):
INSTALL:10: * trytond_purchase_requisition (
Not purchase_request?
File doc/index.rst (right):
doc/index.rst:5: to different suppliers through a wizard that will generate
"…to different suppliers through a selection process."

I think it is implementation detail to talk about wizard.
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.
doc/index.rst:13: Quotation Group
Probably better to name it "Quotation"
doc/index.rst:16: - Supplier: The supplier.
Maybe we need an address to sent the quotation?
doc/index.rst:17: - Quotations:
Name it "Lines"
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.
doc/index.rst:31: - Company: The company which issue the request quotation.
could be defined just after the Supplier
File (right): STATES = [
Try to avoid such global definition. They are not useful after all. ('cancel', 'Canceled'),
canceled also for internal class Group(Workflow, ModelSQL, ModelView):
Just Quotation readonly=True, select=True)
I do not think it is needed to have a select here. request_quotations =
fields.One2Many('purchase.request.quotation', 'group',
Would name it: lines request_quotations =
fields.One2Many('purchase.request.quotation', 'group',
Missing domain on company, supplier. ('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? @staticmethod
Do not use staticmethod for default value.
staticmethod are for constructors. if values.get('number') is None:
Should not the number be filled when sending? class PurchaseRequestQuotation(
QuotationLine sequence_ordered(), ModelSQL, ModelView):
I think it fill in one line. supply_date = fields.Date('Expected Supply Date')
The label should the same as the field name. ondelete='RESTRICT',
I think it should be CASCADE. readonly=True,
Why readonly? ('company', '=', Eval('company', -1)),
Missing supplier fields.Selection(STATES, 'Request for Quotation Group State'),
Better to use a function which takes the states from the group.
Try to keep the same order between field definition and there on_change/default
methods. return
There is no None in the STATES. if (self.unit_price is None) or (self.quantity is None):
parenthesis are not needed. 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. def search_group_state(cls, name, clause):
should be defined close to the getter requests = fields.Many2Many('purchase.request', None, None,
What is the point to show the requests as they are those that were selected. __name__ = 'purchase.request.create_quotation'
I think it is better named:

purchase.request.quotation.create 'purchase_request_quotation.' +
No need of '+' Button('Process', 'process', 'tryton-go-next', default=True),
I think it should be tryton-ok as it is the final step. process = StateTransition()
As it is mainly about creation, it should be named create_ self.raise_user_warning(str(request), 'previous_quotation', {
warning should not be raised on default method but on transition. def transition_start(self):
should be defined before the default_ask_suppliers ('id', 'in', request_ids),
As the size of request_ids could be very large, I think it is better to filter
in Python. return 'end'
I think the user need to have a feedback in this case.
Maybe an error message. 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. new_quotations.append({
you should create an instance instead of dict, it is easier to manipulate. })
This creation should be customable, the best way is to have a method that return
new instance using the request as parameter. for supplier in getattr(self.ask_suppliers, 'suppliers', []):
idem about silent failure if suppliers:
suppliers is required so no need to check it. for (nq, s) in product(new_quotations, suppliers):
The product could be done a the list of request and suppliers. key=self._group_purchase_request_quotation_key)
Instead of creating and after grouping. Is it not better to create already
you could have a loop over supplier and inside loop over the request to create
one quotation with all the lines. active_quotations = fields.Function(fields.One2Many(
Better to move the prefix as suffix: quotations_active
This creates a sort of namespace. quotations = [q for q in self.active_quotations]
What is the point of active_quotations?
Is it not better to have quotations_answered? quotations = sorted(quotations, key=keyfunc)
I think it is better to just define an order on the quotations field. state = 'quotation'
Should not we have a state which says if there is at least one answered
And maybe a deadline on the quotation request. cls.write(requests, {
could be written:

if value:
   cls.write(…) if quotation.group_state in ('draft', 'sent', 'answered'):
use set for in test, more logical:

… in {…} 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. reqs =[
better to use full name: requests

it is not like you do not have place. ('id', 'in', request_ids),
idem about large list.
It is better to loop over instances. })
better to use the pattern:

for req in reqs:
  if …: = … 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
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. key += (('currency', request.best_quotation.currency),)
You should override the currency property of request instead.
File purchase.xml (right):
purchase.xml:7: id="purchase_request_quotation_group_view_tree">
it is a list not a tree.
purchase.xml:80: </record>
What is the point of this access?
purchase.xml:85: </record>
purchase.xml:101: </record>
What is the point of this access?
purchase.xml:129: </record>
Define sequence in a noupdate data block.
purchase.xml:161: <record model=""
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.
purchase.xml:165: </record>
I do not understand those rules.
We just need company rules.
purchase.xml:203: <field name="group"
I'm wondering if we should not have a specific group.
And the other purchase groups will have only read access.
purchase.xml:206: <field name="perm_delete" eval="True"/>
Missing perm_create
purchase.xml:215: <field name="perm_delete" eval="True"/>
purchase.xml:233: <field name="perm_delete" eval="True"/>
purchase.xml:242: <field name="perm_delete" eval="True"/>
purchase.xml:264: </record>
I think we will need a relate from requests to quotations.
purchase.xml:278: <field
should be named quotation.odt or request_quotation.odt
File (right): MODULE2PREFIX = {}
Wrong template for standard module. description=('Tryton The purchase_request_quotation module of '
Do not duplicate the name of the module in the descrition. 'the Tryton application platform.'),
Just use Tryton. author_email='',
should be tracker email.
File tests/ (right):
tests/ from .test_purchase_request_quotation import suite
missing global import try. See the cookiecutter template.
File tests/scenario_purchase_request_quotation.rst (right):
tests/scenario_purchase_request_quotation.rst:11: >>> from import (create_company,
Why not move create_company on the new line also, it will be more readable.
tests/scenario_purchase_request_quotation.rst:163: >>> q =
Quotation.find([('group_state', '=', 'draft')])
Try to use explicit variables.
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:
File tryton.cfg (right):
tryton.cfg:2: version=4.1.0
File view/purchase_request_form.xml (right):
view/purchase_request_form.xml:6: <separator string="Quotations Info"
colspan="4" id="quotation"/>
I would just use "Quotations" as string.
view/purchase_request_form.xml:7: <field name="active_quotations" colspan="4"
widget="many2many" readonly="1"
80 cols
File view/purchase_request_quotation_group_form.xml (right):
view/purchase_request_quotation_group_form.xml:18: <group col="6" colspan="2"
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