Skip to content
This repository has been archived by the owner on Nov 15, 2022. It is now read-only.

Add Tracking Error Details to SDK Response #8

Closed
wants to merge 2 commits into from

Conversation

kdrdmr
Copy link

@kdrdmr kdrdmr commented May 15, 2020

On trackingRequest if awbStatus is Failure webservice return as some information in Condition array. we can use and log this infos. but current master branch doesn't return this with tracking info.


[@mam08ixo]:

Example web service error response (excerpt):

<Status>
    <ActionStatus>No Shipments Found</ActionStatus>
    <Condition>
        <ArrayOfConditionItem>
            <ConditionCode>101</ConditionCode>
            <ConditionData>No Shipments Found for AWBNumber | 9876543210</ConditionData>
        </ArrayOfConditionItem>
    </Condition>
</Status>

The SDK includes the ActionStatus field with its public response but omits the details.

This PR attempts to change that.

@mam08ixo mam08ixo changed the title Return awbConditions with TrackingInfo Add Tracking Error Details to SDK Response May 18, 2020
@mam08ixo
Copy link
Contributor

Hi @kdrdmr, thank you for the contribution.

The classes in Webservice\Soap\Type represent the XML exchange data and are meant for communication between SDK and SOAP web service. They must not be exchanged between SDK and consumer.

I suggest mapping the conditions collection to a simple string array in \Dhl\Express\Webservice\Soap\TypeMapper\TrackingResponseMapper::map and add it to the TrackingInfo public api data type. The result should be something like this:

TrackingInfo::$awbConditions = [
    "[ConditionCode]: [ConditionData]",
    // …
]

@ngolatka ngolatka requested a review from mam08ixo May 19, 2020 08:09
@ngolatka ngolatka added the enhancement New feature or request label May 19, 2020
@kdrdmr
Copy link
Author

kdrdmr commented May 19, 2020

@mam08ixo hi,
thank you for your suggestion. I review it again and I don't show any usage on soap classMap for TrackingInfo or any usage on request side for \Dhl\Express\Webservice\Soap\TypeMapper\TrackingResponseMapper::map method. so I don't see any problem with this usage.

Please let me know clearly; if your comment about only usage \Dhl\Express\Webservice\Soap\Type\Tracking\ConditionCollection Type for return item or not? if I not use this type and return same data in another type[] like string[] or etc.. Is it will be ok?

@mam08ixo
Copy link
Contributor

Yes, my point is that you expose the internal type ConditionCollection to the outside world by including it with the public api TrackingInfoInterface.

Map the additional error information (these AWB conditions) to a native type like string[] and I call it good.

This would still be a backward compatibility breaking change but we'll deal with that when releasing a new version.

@kdrdmr kdrdmr force-pushed the master branch 3 times, most recently from ba0c99f to 638c236 Compare May 20, 2020 08:49
@kdrdmr
Copy link
Author

kdrdmr commented May 20, 2020

Hi again @mam08ixo
I updated code. I hope it is ok now :)
Regards

@kdrdmr
Copy link
Author

kdrdmr commented May 22, 2020

Hi again @mam08ixo ;
is there any problem with current state to prevent merging yet?

@CybotTM
Copy link
Member

CybotTM commented May 23, 2020

Hi again @mam08ixo ;
is there any problem with current state to prevent merging yet?

I think he is just not in the office over the public holiday and the bridge day. :-)

@CybotTM
Copy link
Member

CybotTM commented May 23, 2020

@kdrdmr,

i noticed some minor coding style issues, would be nice if you could fix them. I do not know the exact coding style used here, it seems missing in the README or composer.json - but just take a look at the existing code.

Also your doc for awbConditions seems wrong to me, it should be at least mixed[] instead of just mixed - because it's always an array, isn't it?

Cheers.

@kdrdmr
Copy link
Author

kdrdmr commented May 23, 2020

Hi @CybotTM
I make some updates via your comments. awbConditions changed from mixed[] to key->value array (string[]). I think this is more simple structure than the other.

Greetings

@mam08ixo mam08ixo closed this in ceac287 May 25, 2020
@mam08ixo
Copy link
Contributor

The TrackingInfo entity is not meant to be manipulated after creation. Hence, public properties or setters must not be added. I updated that part and merged the changes.

Thank you for the contribution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants