Semi-automatic ordering of products from suppliers #30
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #30 +/- ##
==========================================
+ Coverage 89.34% 90.1% +0.75%
==========================================
Files 73 74 +1
Lines 2535 2739 +204
Branches 137 148 +11
==========================================
+ Hits 2265 2468 +203
Misses 247 247
- Partials 23 24 +1
Continue to review full report at Codecov.
|
| product.sku, supplier.internal_name) | ||
| else: | ||
| msg = 'Could not order {}.'.format(product.sku) | ||
| raise exceptions.APIException(msg) |
There was a problem hiding this comment.
This else clause should always run, regardless of what the iterable products will be. If I'm not missing any black magic here.
...
I just saw the test and now I'm sure there is some black magic here, as long as the test is not failing. What is the black magic?
If I try to simulate it as follows, the else clause is always executed:
for x in [1,2,3,4,5]:
print("FOR")
else:
print("STILL")
...
FOR
FOR
FOR
FOR
FOR
STILLAlso, do we want to localise the msg string?
UPDATE:
WOW, I completely missed the return statement, no black magic going on. I need some coffee...
There was a problem hiding this comment.
else won't be executed if you do a break in your loop. https://docs.python.org/3/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops
I do not know however how I feel about translating internal exceptions. I know that I'm displaying their messages in some places (exceptions from the stock taking module) in the admin panel, but it feels kind of wrong. I'll think about it and get back to you.
There was a problem hiding this comment.
Yeah, ignore my first section of the comment. I completely missed the return statement
| @@ -87,6 +97,15 @@ class Meta: | |||
| verbose_name_plural = _('supplier products') | |||
| unique_together = ('supplier', 'sku',) | |||
|
|
|||
| @property | |||
| def unit_price(self): | |||
| return self.price / self.qty_multiplier | |||
There was a problem hiding this comment.
It might be unlikely, but couldn't the multiplier end up as 0 here? I didn't see any barriers in the admin model.
|
|
||
| @abstractmethod | ||
| def order_product(self, sku, qty): | ||
| """Places an order on product with given SKU. |
There was a problem hiding this comment.
What is SKU? Please write the full definition at least once before using the abbreviation
There was a problem hiding this comment.
SKU is a well known abbreviation when you dealing with inventory management, but a clarification won't hurt anybody I guess!
There was a problem hiding this comment.
Well, that's a good justification, however it just saves readers trouble to write the full definition once at least.
src/shop/tests/test_api.py
Outdated
| ) | ||
|
|
||
| def order_product(self, sku, qty): | ||
| pass |
There was a problem hiding this comment.
Is this a drive-by test? Or maybe just forgotten?
There was a problem hiding this comment.
It's just something we need to do as long we keep DummySupplierAPI around. All the methods in SupplierBase are decorated with @abstractmethod and all the children classes need to implement all those methods, otherwise hell will break loose and Python won't run your code.
There was a problem hiding this comment.
order_product is tested using another mocking method and we could probably do the same for retrieve_product and parse_delivery_report. Then, that dummy class could be removed entirely.
There was a problem hiding this comment.
I see, yeah maybe a small refactor could be in order.
| price=1, | ||
| units=1 | ||
| ) | ||
| sp5 = api.order_from_supplier(product1.id, 48) |
There was a problem hiding this comment.
In my eyes, there is some black magic going on here, this should raise an exception.
There was a problem hiding this comment.
Because of my first comment. I have soon finished my coffee and hope the embarrassment goes away soon.
For every product one can define base stock level (the desired quantity). Then, when placing an order from a supplier, system will aid by suggesting which products should be ordered, based on the current demand. In case of Narlivs, the "aiding" process will be that the system will prefill the cart at Narlivs with the items that should be ordered.