karrioapi / karrio

Programmable Shipping API (self-hosted)
https://karrio.io
Apache License 2.0
460 stars 91 forks source link

Canadapost cancel error (Void) is not behing catched #682

Open jacobshilitz opened 6 days ago

jacobshilitz commented 6 days ago

Describe the bug Canada post does not catch a void cancel error with empty message

<?xml version="1.0" encoding="UTF-8"?>

<messages xmlns="http://www.canadapost.ca/ws/messages"/>

as the docs say it return 404. I can't see the status code to be passed to the parse_error_response method

in addtion, we need to check if a menifest was already ishued, and if yes we need to do a shipmentrefund becouse a voidshipment is not allowed after a manifest was issued

Wouldn't checking status codes be a good practice?

danh91 commented 5 days ago

I believe at the API level, once you have a manifest attached to a shipment it doesn't allow cancelling the shipment 🤔.

This one might require some API logs or a way to reproduce to debug it because the current implementation is supposed to cover the case described above:

https://github.com/karrioapi/karrio/blob/5b235d94ca02495a7d3996425653ac9b4b10b56c/modules/connectors/canadapost/karrio/mappers/canadapost/proxy.py#L106-L192

jacobshilitz commented 5 days ago

You are right, i missed this one

i rechecked the code, and it seems that the problem is that the api should send shipment-id and not the tracking code

<shipment-id>25208172608**9372</shipment-id>
<shipment-status>created</shipment-status>
<tracking-pin>80464771409**36</tracking-pin>

i think the bug is here, and that shipment_identifier should save shipment_id not the tracking pin https://github.com/karrioapi/karrio/blob/692efcc9f86ed6a674f191f7b1f92ebc351e1f52/modules/connectors/canadapost/karrio/providers/canadapost/shipment/create.py#L50-L51

see PR #683

in any case the 404 with empty message was not catched as a error (there was 2 404 in a row)

jacobshilitz commented 5 days ago

the second bug which I don't know how to fix or if it needs a fix, that when you cancel on the dashboard, how do you pass a email?

also it should throw a error if no email, because it's a required field

maybe we should have a special setting in the carrier level. https://github.com/karrioapi/karrio/blob/692efcc9f86ed6a674f191f7b1f92ebc351e1f52/modules/connectors/canadapost/karrio/providers/canadapost/shipment/cancel.py#L52-L52

jacobshilitz commented 5 days ago

the third bug is that after hardcoding my email it did open a ticket, but the cancel step still tried to run.

i'm emailing you the logs

jacobshilitz commented 5 days ago

I think i found one of the bugs when you try to cancel and refund request was done the parsing is buggy with 2 bugs.

first here is a successful refund reply

<?xml version="1.0" encoding="UTF-8"?>
  <shipment-refund-request-info xmlns="http://www.canadapost.ca/ws/shipment-v8">
  <service-ticket-date>2024-09-11</service-ticket-date>
  <service-ticket-id>015***7353</service-ticket-id>
</shipment-refund-request-info>

but this code that is checking if it was refunded is checking a condition that never happen

https://github.com/karrioapi/karrio/blob/36217a0040467240ceba5cc42a073b9a64f37109/modules/connectors/canadapost/karrio/mappers/canadapost/proxy.py#L181-L184

something is wrong with this logic because when you run this python code manually it return a string {http://www.canadapost.ca/ws/shipment-v8}shipment-refund-request-info not shipment-refund-request-info (let me know how we fix this, or if we test against the full url)

getattr(lib.to_element('<?xml version="1.0" encoding="UTF-8"?><shipment-refund-request-info xmlns="http://www.canadapost.ca/ws/shipment-v8"><service-ticket-date>2024-09-11</service-ticket-date><service-ticket-id>0157**53</service-ticket-id></shipment-refund-request-info>'), "tag", None)

bug 2 in there is a case where the refund was already done, and at that case we also don't need to void, but rather return this error to the user, not a false error that the shipment is not in good status

The response when you refund a order that already had a refund request

<?xml version="1.0" encoding="UTF-8"?>
  <messages xmlns="http://www.canadapost.ca/ws/messages">
    <message>
      <code>7292</code>
      <description>A refund has already been requested for this shipment.  Note that refund requests may take a few days to be processed.</description>
    </message>
  </messages>
jacobshilitz commented 5 days ago

I've been playing around with the code, and I think I've fixed the problems so far. However, I'm afraid you won't like my solution, and I'm sure you'll have a better way to achieve the same result.

                    refunded=(
                        "shipment-refund-request-info" not in getattr(lib.to_element(response), "tag", None)
                        and "messages" not in getattr(lib.to_element(response), "tag", None)
                    ),

but it's not so good because code 7291 means you should use void, so we should just check if its 7292 and 7296

image