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

OMG-25427 - Set SHIPPO-ACCOUNT-ID header #7

Merged
merged 22 commits into from
Sep 18, 2024
Merged

Conversation

slavick
Copy link
Member

@slavick slavick commented Sep 5, 2024

OMG-25427

Update to support the SHIPPO-ACCOUNT-ID header required for Shippo's new sub-account system.

Related PR:

client/client.go Outdated Show resolved Hide resolved
Copy link

@ilyutov ilyutov left a comment

Choose a reason for hiding this comment

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

Why don't we add a separate "headers" argument to Client.do instead of changing every struct definition?

@permanuno permanuno self-requested a review September 11, 2024 15:54
@slavick
Copy link
Member Author

slavick commented Sep 11, 2024

Why don't we add a separate "headers" argument to Client.do instead of changing every struct definition?

Good idea! I made the changes in this PR and am in the process of updating the shipping-service PR.

Copy link

@ilyutov ilyutov left a comment

Choose a reason for hiding this comment

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

👍

client/client.go Outdated
// add any passed in headers
if headers != nil {
for k, v := range headers {
c.logPrintf("Client.createRequest() setting header: key=%q, value=%q", k, v)
Copy link
Member

Choose a reason for hiding this comment

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

might want to relocate the c.logPrintf to the defer func above, looks like logging is only done if the logger isn't nil

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, but logPrintf also checks if logger is nil.

Copy link
Member

@permanuno permanuno left a comment

Choose a reason for hiding this comment

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

i think GetTrackingUpdate needs an update.

@@ -8,7 +8,7 @@ import (
)

// GetTrackingUpdate requests the tracking status of a shipment.
func (c *Client) GetTrackingUpdate(carrier, trackingNumber string) (*models.TrackingStatus, error) {
func (c *Client) GetTrackingUpdate(carrier, trackingNumber string, shippoSubAccountID string) (*models.TrackingStatus, error) {
Copy link
Member

Choose a reason for hiding this comment

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

i don't think we'll need subaccount here, there's a carrier but it's usps not the carrier id: https://docs.goshippo.com/shippoapi/public-api/#operation/GetTrack

@permanuno permanuno requested a review from ilyutov September 12, 2024 19:55
@slavick slavick marked this pull request as ready for review September 17, 2024 20:53
@slavick
Copy link
Member Author

slavick commented Sep 17, 2024

Why don't we add a separate "headers" argument to Client.do instead of changing every struct definition?

@ilyutov This is the strategy we went with. Please review when you have a chance.

Copy link

@ilyutov ilyutov left a comment

Choose a reason for hiding this comment

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

LGTM

@slavick slavick merged commit 44c1ed5 into master Sep 18, 2024
@slavick slavick deleted the slavick/OMG-25427 branch September 18, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants