Conversation
dreispt
changed the title
dreispt
force-pushed
the
19-dr-delivery_dpd_portugal
branch
from
ae0ee4c to
54d30d8
Compare
dreispt
force-pushed
the
19-dr-delivery_dpd_portugal
branch
from
54d30d8 to
d41bdae
Compare
| selection_add=[("dpd_portugal", "DPD Portugal")], | ||
| ondelete={"dpd_portugal": "set default"}, |
There was a problem hiding this comment.
Personally I like the country code more than the the country name:
| selection_add=[("dpd_portugal", "DPD Portugal")], | |
| ondelete={"dpd_portugal": "set default"}, | |
| selection_add=[("dpd_pt", "DPD Portugal")], | |
| ondelete={"dpd_pt": "set default"}, |
Less to write on each new field and method.
The name of the module could be changed too, but not necessary. Not everyone knows the country code. But if you are inside the module, you will understand that PT = Portugal
| dpd_portugal_prod_environment = fields.Boolean( | ||
| string="Production Environment", | ||
| default=False, | ||
| help="Use production API instead of test environment", | ||
| ) |
There was a problem hiding this comment.
I think you can replace this with standard field prod_environment. Your field sounds like as it does the exact same
| if self.delivery_type != "dpd_portugal": | ||
| return super().rate_shipment(order) |
There was a problem hiding this comment.
| if self.delivery_type != "dpd_portugal": | |
| return super().rate_shipment(order) |
dpd_portugal_rate_shipment is called inside of rate_shipment:
https://github.com/odoo/odoo/blob/35bc748256de1b5eac81007a62d43dc663726ab6/addons/delivery/models/delivery_carrier.py#L322
It is a carrier-specific implementation. dpd_portugal_rate_shipment will always be called only for carriers that have dpd_portugal as delivery_type. So the check is not necessary either
| if self.delivery_type != "dpd_portugal": | ||
| return super().send_shipping(pickings) | ||
|
|
||
| self.ensure_one() |
There was a problem hiding this comment.
Same as with rate_shipment.
ensure_one is done by send_shipping, no need to do it here again:
https://github.com/odoo/odoo/blob/35bc748256de1b5eac81007a62d43dc663726ab6/addons/stock_delivery/models/stock_picking.py#L126
| if self.delivery_type != "dpd_portugal": | |
| return super().send_shipping(pickings) | |
| self.ensure_one() |
| attachment = self.env["ir.attachment"].create( | ||
| { | ||
| "name": ( | ||
| f"DPD_Portugal_Label_{tracking_number}" | ||
| f".{self.dpd_portugal_label_format.lower()}" | ||
| ), | ||
| "type": "binary", | ||
| "datas": base64.b64encode(label_content), | ||
| "res_model": "stock.picking", | ||
| "res_id": picking.id, | ||
| } | ||
| ) | ||
| picking.message_post( | ||
| body=self.env._("DPD Portugal label generated"), | ||
| attachment_ids=[attachment.id], | ||
| ) |
There was a problem hiding this comment.
Something I learned recently: message_post can create attachments on its own. Here in delivery_schenker it is implemented like this:
delivery-carrier/delivery_schenker/models/delivery_carrier.py
Lines 467 to 473 in 2630fc8
If you want you can try it. Less code to write.
You are using the attachment later. Maybe message_post somehow returns the ID of the created attachment. If not, keep your code
|
|
||
| def test_supports_shipping_insurance(self): | ||
| """Test insurance support.""" | ||
| self.assertTrue(self.dpd_carrier.supports_shipping_insurance) |
There was a problem hiding this comment.
This is useless. The check below is fine
| invalid_partner, "recipient" | ||
| ) | ||
|
|
||
| def test_api_url_selection(self): |
There was a problem hiding this comment.
Not really useful. Remove it
| } | ||
| ) | ||
|
|
||
| self.assertEqual(picking.dpd_portugal_cod_amount, 150.0) |
There was a problem hiding this comment.
The field is not computed, it is set a few line above it. Useless check, remove it
|
|
||
|
|
||
| @tagged("post_install", "-at_install") | ||
| class TestDeliveryDPDPortugal(TransactionCase): |
There was a problem hiding this comment.
There are no unit tests that check the actual carrier API functionality like tracking state and shipment creation.
You can mock requests.post to prevent that the API is called, you can modify the result to raise HTTP errors and you can check the JSON data that was passed to post, so the shipment data and tracking data.
Please add tests to cover these cases. Try the API, check in which cases the API returns an error and re-create this behaviour with the mocked post to test how the module behaves.
With these tests everyone of us can execute the tests to know whether the main part of your implementation actually works as we do not need credentials for the API
| <field name="inherit_id" ref="delivery.view_delivery_carrier_tree" /> | ||
| <field name="arch" type="xml"> | ||
| <xpath expr="//field[@name='delivery_type']" position="after"> | ||
| <field name="dpd_portugal_service_type" optional="show" /> |
There was a problem hiding this comment.
Why do you want to show the field in the carrier list view? Remove it or make it optional="hide"
dreispt
force-pushed
the
19-dr-delivery_dpd_portugal
branch
from
d41bdae to
5a34942
Compare
OCA-git-bot
added
series:19.0
mod:delivery_dpd_portugal
|
@raumschmiede-joshuaL Thank you for the thorough review. |
3 participants
No description provided.