Add tracking api #19
Add tracking api #19
Conversation
Shippo/APIResource.cs
Outdated
|
|
||
| public Track RetrieveTracking (String carrier, String id) | ||
| { | ||
| string ep = String.Format ("{0}/tracks/{1}/{2}", api_endpoint, carrier, id); |
There was a problem hiding this comment.
Could you encode carrier and id in case they end up having reserved characters?
ShippoTesting/TrackTest.cs
Outdated
| public void TestValidGetStatus () | ||
| { | ||
| Shipment shipment = ShipmentTest.getDefaultObject (); | ||
| Track track = getAPIResource ().RetrieveTracking ("usps", shipment.ObjectId); |
There was a problem hiding this comment.
+1 for taking shipment ID from the Shipment.getDefaultObject(). I had hardcoded in Java. Will do the same there.
There was a problem hiding this comment.
Wait I just realized doesn't this take tracking number given by the carrier. Instead takes shipment ID from shippo. If so, how is this test working?
There was a problem hiding this comment.
Does this mean the test never worked? If so, could you please avoid requesting a review before completely testing it? My assumption before reviewing is that the code has been completed and tested. Let me know if this is ok with you.
Shippo/Track.cs
Outdated
| [JsonObject (MemberSerialization.OptIn)] | ||
| public class Track : ShippoId { | ||
| [JsonProperty (PropertyName = "carrier")] | ||
| public object Carrier { get; set; } |
There was a problem hiding this comment.
Since tracking information is primarily for consumption do you think its best to leave out setters?
There was a problem hiding this comment.
We need the getters for it to be initially filled out, else they will always be NULL
There was a problem hiding this comment.
Do you mean to say that JsonProperty needs the setters? If so, then its good. Interestingly Java's GSON doesn't need the setters. It is able to set private variables using Java reflection.
I also just realized that all the members are public. They should probably be private with getter/setter. The getter/setter are not required if they are public.
There was a problem hiding this comment.
Just tried changing them to private, it doesn't allow me to access the properties. Instead I just removed the getters and setters and tests pass 👍
There was a problem hiding this comment.
They probably pass because attributes are still public. Can you try making them private with only getter? If this works, could you please do the same for TrackingEvent and TrackingHistory? Thanks!
There was a problem hiding this comment.
Just tried changing them to private with 'get', but it doesn't work
ShippoTesting/TrackTest.cs
Outdated
| Shipment shipment = ShipmentTest.getDefaultObject (); | ||
| Track track = getAPIResource ().RetrieveTracking ("usps", shipment.ObjectId); | ||
| Assert.IsNotNull (track.TrackingNumber); | ||
| Assert.IsNotNull (track.TrackingHistory); |
There was a problem hiding this comment.
Asserting these is duplicate effort already done in TestValidGetStatus. I think it can be skipped.
|
And thank you very much for splitting the PRs. It is really helpful. |
ShippoExample/Example.cs
Outdated
|
|
||
| private static void RunTrackingExample (Shipment shipment, APIResource resource) | ||
| { | ||
| Track track = resource.RetrieveTracking ("usps", shipment.ObjectId); |
There was a problem hiding this comment.
tracking number instead of shipment.ObjectId
|
Thank you so much for adding types to the class. It makes it way better. I left a comment about public attributes and shipmentID being used in example. Could you please address those? Once done I think this is good to go :) |
|
Created #20 for immutable classes. Without that this LGTM 👍 |
No description provided.