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

GLS FR via REST Web Service #138

Merged
merged 4 commits into from
Mar 18, 2021
Merged

GLS FR via REST Web Service #138

merged 4 commits into from
Mar 18, 2021

Conversation

DylannCordel
Copy link
Contributor

@DylannCordel DylannCordel commented Jan 13, 2021

based on #137

Tests included except for SHD service: with the parcelshopid from the doc, GLS returns a Server Error. I'll ask them a valid parcelshopid.

Some changes have beend done inside the base transport.py to be able to use any requests args required by the web service we are using.

@DylannCordel DylannCordel force-pushed the gls_eu branch 2 times, most recently from 14b7880 to 98e9b0f Compare January 20, 2021 16:23
@DylannCordel
Copy link
Contributor Author

Checks failed due to GLS or Travis network issue... @florian-dacosta could you force a restart of the travis job please ? Then, this PR and #139 are ready to be reviewed.

Copy link
Member

@florian-dacosta florian-dacosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this work @DylannCordel

I am not sure I understand the gls_eu thing, it is still a specific french GLS webservice right ?
eu make me think of europe and confuse me, should this work for the GLS in any european country?
It does not seem so.

From what I understand, it is, as gls_fr which manage the "GLS BOX", only for the french GLS and it seems it can't even replace gls_fr which deliver zpl, as the only format of gls_eu are pdf and png.
Is that right ?

Anyway, we'll be able to merge this, but I first want to make sure I understand the difference between the 2 gls and see if an other suffix would be appropriate or not, etc...
Thanks in advance for your enlightenment !

roulier/api.py Outdated
"required": True,
"empty": False,
},
"shippingDate": {"type": "date", "required": True, "empty": False,},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to be extremely careful with this kind of changes.
It is not harmless, changing this type, which was a string until now, will make all external app break as they will be sending the type in the wrong format after the merge of this PR...
It would be nice to announce these breaking changes in the description of the PR IMHO

Since this python3 roulier versions is still recent and no wildly used yet, I guess I'll be able to accept the change, but this kind of breaking change will be harder to change in the future, we will need to keep compatibility with previous behaviors.
But I agree, it is better for roulier to receive a value in date format.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is based on the #137 which is specifically the change of the date format with explanation about it ;).
I didn't want to push this one (#138) with a string date, then update it when #137 would be merged to use a real date. I was quite sure you'll accept the PR #137 as it's for standardization. Currently the shippingDate param doesn't use the same format between existing roulier's carriers API (same type yes 'string) but not format)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, I missed #137 indeed !
Ok, I'll merge 137 very soon then, thanks

@DylannCordel
Copy link
Contributor Author

DylannCordel commented Feb 11, 2021

Hi @florian-dacosta

Thx for feed-back. I named this carrier because of the API URL : api.gls-group.eu. I must admit I don't know if Deutch GLS instance for exemple use the same API.

@florian-dacosta
Copy link
Member

@DylannCordel FYI, I've ask to GLS support and they did tell me that this solution WEB Api is only for France, just as the GLS-BOX.

@DylannCordel
Copy link
Contributor Author

Too bad. Thanks for the feed-back. Do you want I rename it now as gls_fr_rest or do we wait to know what is decided in #140 ?

@florian-dacosta
Copy link
Member

Too bad. Thanks for the feed-back. Do you want I rename it now as gls_fr_rest or do we wait to know what is decided in #140 ?

I think you can rename it already yes.
Actually, if we just add a suffix for the implementation, we don't even need to change base roulier, so we can switch each carrier independently.

Also, what do you think about file organisation?
We could have one folder gls
and one subfolder gls_fr_rest, gls_fr_glsbox.
It would be easier no?

Indeed, I guess if a carrier propose multiple implementation, we can hope it to expects more or less the same data, so some class could eventualy be shared (mainly the api class)

@DylannCordel
Copy link
Contributor Author

Reorganization done.

@DylannCordel
Copy link
Contributor Author

Indeed, I guess if a carrier propose multiple implementation, we can hope it to expects more or less the same data, so some class could eventualy be shared (mainly the api class)

You right, I'll see if we can refactor some code to share a base API class for both GLS.

@DylannCordel DylannCordel changed the title GLS eu via REST Web Service GLS FR via REST Web Service Feb 22, 2021
@florian-dacosta
Copy link
Member

@DylannCordel

One remark : I could not use incoterm using gls_fr_rest, trying any value among https://github.com/akretion/roulier/pull/138/files#diff-6a238661790fccc1ae250d3a0c8f16c3efc31df8f18b7d1a3ee3612e8e481739R57
I always have an error from gls webservice... I only could create label without passing incoterm, but is a problem for shipping in foreign countries.
Also, I am quite surprise that there is a mix of abbreviation and code in the allowed value.

Anyway, it is not blocking for now I think because it is a new service and already usable for a lot of case (cases with no incoterm).
It would be nice to fix/check this but we can merge in the meanwhile.

Can you resolve the conflict ?
(Sorry I made a small change is glsbox zpl template some time ago) I'll merge as soon as it is done.

Thanks again for your work. From what I understood, Gls will implement ZPL format soon on the rest api.
Once they do, I'll switch to this one.

@DylannCordel
Copy link
Contributor Author

Hi @florian-dacosta

I fix the icoterm validation (my mistakes, sorry, due to a bad copy/paste/clean from the doc). Now, it's just a string because I'm waiting for an answer from the gls support team because I confirm the icoterm field is not usable at this stage but I don't know why (missing other undocumented data ? wrong values ?)

I rebased and added three tests for icoterm :

  • first one tests with valid data and icoterm. It's temporay disabled because of reasons above.
  • second is for wrong icoterm (enabled and working, even with a valid icoterm ahah)
  • third is for a missing icoterm (enabled and working)

So you can merge it now if you want or wait for the answer of GLS support.

@florian-dacosta
Copy link
Member

Any new from GLS support ?

@florian-dacosta
Copy link
Member

@DylannCordel
I guess it would be nice to add a known issue kind of section in readme to detail, by carrier the possible problem (like now the incoterm for gls).
Well, since it is a new carrier and all seems fine beside this incoterm issue, I am merging it.

But, I am still interested to know about the response of GLS support when you'll have it!
Thanks again for this work!

@florian-dacosta florian-dacosta merged commit ba5640c into akretion:master Mar 18, 2021
@DylannCordel
Copy link
Contributor Author

Yes they answered me but I didn't have the time to fix it yet. Documentation is wrong: field must not be in the address, but at the root level:

{
    "shipperId": "2505400034 250aaawbn6",
    "addresses": {
        "delivery": {
            "name1": "Pricne",
            "street1": "Place de Bel-Air",
            "country": "CH",
            "zipCode": "1204",
            "city": "Geneve"
        }
    },
    "incoterm": 20,
    "parcels": [{
        "weight": 2.5
    },
    {
        "weight": 2.5
    }],
    "labelFormat" : "PDF"
}

I opened an issue #143 to not forget to fix it. You can assign it to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants