Set picking type on PO generated from sourcing with FA #86
Conversation
|
please add a couple of automated tests |
This bridge with onchanges on purchase_delivery_address and make it consistant with purchase_requisition_delivery_address
yvaucher
force-pushed
the
8.0-fix-fa-po-picking-type
branch
from
4c7a964
to
dfd288d
Compare
yvaucher
force-pushed
the
8.0-fix-fa-po-picking-type
branch
from
dfd288d
to
f4262c8
Compare
|
@gurneyalex Tests added |
| @@ -52,9 +52,80 @@ def setUp(self): | |||
| self.other_source = next(x for x in self.source_lines | |||
| if x.procurement_method == 'other') | |||
|
|
|||
| def test_01_transform_source_to_agreement(self): | |||
| def test_01_transform_source_to_agreement_wh_dest(self): | |||
There was a problem hiding this comment.
I think good practices advices to have small tests with one scenrio/case by test method
There was a problem hiding this comment.
@foutoucour can you please elaborate more?
IMO we can have both
- "real" isolated unit tests (setup - run one method - assert on the result)
- integration/story-like tests
In the core there are only integration ones, but that doesn't mean we shouldn't write isolated ones. I personally am for isolation wherever possible, but I'd rather avoid that big debate between isolation and integration and accept both styles.
There was a problem hiding this comment.
If there is parts we can reuse we should at lest split the block. It will ease redability
There was a problem hiding this comment.
@nbessi indeed nesting helper methods helps to DRY, but sometimes I personally find it hard to follow and somewhat verbose (less repetition but more boilerplate).
Instead often I prefer to have shorter, more expressive test by splitting them in functionally significant parts (small realistic scenarios instead of pages-long stories), and avoiding data irrelevant to the test (sometimes with help from Model.new() and mocks). A bit of functionally relevant repetition in an expressive, short test is fine by me.
This is 100% subjective preference, of course: we all just want to make our tests readable and maintainable.
There was a problem hiding this comment.
I meant if it is a unittest I prefer to have one test by assert. The tests might be long at the end of the day but it is really helpful during debug I found.
I didn't realised that was not the case here and I understand why the test has so many asserts.
|
There are two tests numbered 01. Each one is run in its own transaction, so I don't think we need to number them at all. Otherwise 👍 thanks! |
|
@foutoucour do you think we can merge this one? If not, what do you suggest? Thanks! |
|
👍 |
| # if destination is not for a warehouse address, | ||
| # we set dropshipping picking type | ||
| ref = 'stock_dropshipping.picking_type_dropship' | ||
| picking_type_id = self.env['ir.model.data'].xmlid_to_res_id(ref) |
There was a problem hiding this comment.
We might want to use env.ref and return a record directly?
|
👍 |
Set picking type on PO generated from sourcing with FA
This bridge with onchanges on purchase_delivery_address
and make it consistant with purchase_requisition_delivery_address