Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ADD] [15.0] delivery_dhl_de_oca #141

Closed
wants to merge 3 commits into from
Closed

[ADD] [15.0] delivery_dhl_de_oca #141

wants to merge 3 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 29, 2023

We are currently implementing several API's for DHL namely

Please ignore fileheaders / license, formating and missing tests at the moment, but would warmly welcome feedback on the approach.
The attached code is working with the appropriate test credentials.

nikolausweingartmair added 3 commits December 29, 2023 17:42
@fkantelberg
Copy link
Member

Hello,

I can give you some feedback about your modules:

  • It's unusual to store the credentials on res.company. Usually you store them on delivery.carrier or you can look into carrier.account of base_delivery_carrier_label This also solves the problem with many new fields on res.company for the different service (see also below).
  • Why not use lxml for xml processing instead of introducing new dependencies? It's also possible to use qweb to render the outgoing xml if lxml is unhandy
  • Why are there multiple modules? You could use a field on delivery.carrier to select the different services of the API instead of 3 modules.
  • The tracking.event model isn't generic and should be renamed dhl.tracking.event or pushed into a generic module to be used by other modules too. I know that this wouldn't be the only delivery carrier which returns tracking events and different models/views are hard to handle in the long term.
  • This is only for delivery within germany right otherwise the repository wouldn't be great.
  • You don't have to handle the base64 for the HTTP auth. You can use HTTPBasicAuth of requests instead.

Best regards

@ghost
Copy link
Author

ghost commented Jan 25, 2024

@fkantelberg:

Thanks for taking the time to review the modules:

A) Credentials in delivery.carrier instead of company
I will have a look at carrier.account
B) lxml instead of python dependencies.
The module for Shipping is originally from Vraja (but it's AGPL-3). They did it that way.
I will have a look to refactor it to lxml.
C) Split into 3 different modules:
All these three different modules have different API's although you can ran them as one "APP" in the DHL Management console. You also could register them es 3 different apps. Additionally for maintainance and code encapsulation I wanted to make it separate modules.
D) non generic: tracking.event
Agree, it's very dhl will rename it to dhl.tracking.event.
E) Repo (delivery_carrier <-> l10n-germany)
I also think delivery_carrier would be a better fit (as there are also other county specific modules in there).
But DHL has other API's for international transports and these API's are only vor dhl parcel which is only for germany. So it's fine by me.
E) HTTPBasicAuth
I will have a look at it.

@jans23
Copy link

jans23 commented Feb 23, 2024

I tested these modules but when creating a shipping method and selecting any "Delivery Product" the following error is thrown.

grafik

RPC_ERROR
Odoo Server Error
Traceback (most recent call last):
  File "/srv/odoo/odoo/parts/odoo/odoo/models.py", line 5395, in _update_cache
    field_values = [(fields[name], value) for name, value in values.items()]
  File "/srv/odoo/odoo/parts/odoo/odoo/models.py", line 5395, in <listcomp>
    field_values = [(fields[name], value) for name, value in values.items()]
KeyError: 'dhl_parcel_de_provider_package_id'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/srv/odoo/odoo/parts/odoo/odoo/addons/base/models/ir_http.py", line 237, in _dispatch
    result = request.dispatch()
  File "/srv/odoo/odoo/parts/odoo/odoo/http.py", line 698, in dispatch
    result = self._call_function(**self.params)
  File "/srv/odoo/odoo/parts/odoo/odoo/http.py", line 368, in _call_function
    return checked_call(self.db, *args, **kwargs)
  File "/srv/odoo/odoo/parts/odoo/odoo/service/model.py", line 94, in wrapper
    return f(dbname, *args, **kwargs)
  File "/srv/odoo/odoo/parts/odoo/odoo/http.py", line 357, in checked_call
    result = self.endpoint(*a, **kw)
  File "/srv/odoo/odoo/parts/odoo/odoo/http.py", line 921, in __call__
    return self.method(*args, **kw)
  File "/srv/odoo/odoo/parts/odoo/odoo/http.py", line 546, in response_wrap
    response = f(*args, **kw)
  File "/tmp/addons/web/controllers/main.py", line 1324, in call_kw
    return self._call_kw(model, method, args, kwargs)
  File "/tmp/addons/web/controllers/main.py", line 1316, in _call_kw
    return call_kw(request.env[model], method, args, kwargs)
  File "/srv/odoo/odoo/parts/odoo/odoo/api.py", line 471, in call_kw
    result = _call_kw_multi(method, model, args, kwargs)
  File "/srv/odoo/odoo/parts/odoo/odoo/api.py", line 456, in _call_kw_multi
    result = method(recs, *args, **kwargs)
  File "/srv/odoo/odoo/parts/odoo/odoo/models.py", line 6465, in onchange
    record = self.new(initial_values, origin=self)
  File "/srv/odoo/odoo/parts/odoo/odoo/models.py", line 5755, in new
    record._update_cache(values, validate=False)
  File "/srv/odoo/odoo/parts/odoo/odoo/models.py", line 5397, in _update_cache
    raise ValueError("Invalid field %r on model %r" % (e.args[0], self._name))
Exception

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/srv/odoo/odoo/parts/odoo/odoo/http.py", line 654, in _handle_exception
    return super(JsonRequest, self)._handle_exception(exception)
  File "/srv/odoo/odoo/parts/odoo/odoo/http.py", line 301, in _handle_exception
    raise exception.with_traceback(None) from new_cause
ValueError: Invalid field 'dhl_parcel_de_provider_package_id' on model 'delivery.carrier'

@jans23
Copy link

jans23 commented Apr 3, 2024

@nikolausweingartmair Any update on the above feedback?

@ghost
Copy link
Author

ghost commented Apr 5, 2024

Hi, jans23

I was on parental leave for 2 months so I'm a little behind things - I hope to have a look at it on the weekend.

Copy link

github-actions bot commented Aug 4, 2024

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Aug 4, 2024
@github-actions github-actions bot closed this Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants