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.

Sign up for GitHub

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

Jump to bottom

[feat] Consolidate SmartRate functions, add new functions #566

Merged
merged 17 commits into from
Jul 16, 2024
Merged

[feat] Consolidate SmartRate functions, add new functions #566

merged 17 commits into from
Jul 16, 2024

Conversation

nwithan8
Copy link
Contributor

@nwithan8 nwithan8 commented May 23, 2024 •
edited
Loading

Description

  • Add new SmartRate service for interacting with the SmartRate API
    • New EstimateDeliveryDate function to estimate delivery date based on a list of carriers, to/from ZIP codes and a planned ship date (no existing shipment required).
    • New RecommendShipDate function to to recommend ship date based on a list of carriers, to/from ZIP codes and a planned ship date (no existing shipment required).
  • New EstimateDeliveryDate function in Shipment service to estimate delivery date for a shipment based on a planned ship date.
  • New RecommendShipDateForShipment function in Shipment service to recommend ship date for a shipment based on a desired delivery date.
  • New model classes as needed for JSON response to new API functions

Testing

  • Unit tests added, updated accordingly
  • Cassettes added, re-recorded accordingly

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

@nwithan8 nwithan8 requested a review from a team May 23, 2024 22:37
Justintime50
Justintime50 reviewed Jun 3, 2024
View reviewed changes
Copy link
Member

@Justintime50 Justintime50 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 this looks good. As discussed, we'll hold off until this goes GA to merge in and release. I feel splitting SmartRate to its own service is a good idea especially as the product matures and more functionality becomes available. Since everything is static now, it doesn't make sense to keep it in the Shipment service because you have to pass in the shipment ID anyway now, it no longer inherently belongs to Shipment.

@nwithan8 nwithan8 requested review from Justintime50 and jchen293 June 4, 2024 21:48
jchen293
jchen293 reviewed Jun 10, 2024
View reviewed changes
Copy link
Contributor

@jchen293 jchen293 left a comment

Choose a reason for hiding this comment

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

Code seems good, just two questions. I will approve when the endpoints are in GA

@Justintime50 Justintime50 marked this pull request as draft June 13, 2024 21:05
@nwithan8 nwithan8 changed the title [feat] Consolidate SmartRate functions, add recommended ship date function [feat] Consolidate SmartRate functions, add new functions Jul 3, 2024
@nwithan8 nwithan8 marked this pull request as ready for review July 3, 2024 05:06
@Justintime50
Copy link
Member

I'd like this PR to follow the conventions set in the Python one so the experience is consistent across libs, namely the location of the functions in two separate services (keeping the existing functions in the Shipment service), renaming them, and not overloading the functions even though that's a feature of this language (to ensure consistency).

@nwithan8 nwithan8 force-pushed the precision_shipping branch from 0e0f589 to 04e19a2 Compare July 11, 2024 20:31
jchen293
jchen293 previously requested changes Jul 12, 2024
View reviewed changes
nwithan8 added 14 commits July 15, 2024 14:41
@nwithan8
- Add endpoint to get recommended ship date based on desired delivery…
9d542b5 
… date

- Clarify existing endpoint to get estimated delivery date based on ship date
- Consolidate SmartRate-related functions into new SmartRate service
- Mark legacy SmartRate functions and classes with obsolete warnings
- Update unit tests accordingly
@nwithan8
- Record cassettes
5de1e99
@nwithan8
- Remove helper function for ship date estimate
b0e9da6
@nwithan8
- Update CHANGELOG
d8675b9
@nwithan8
- Linting and integration test for new classes
5868571
@nwithan8
- Address feedback
f533ee6
@nwithan8
- Add Smart Deliver By and Smart Deliver On API support (estimate del…
d015639 
…ivery date or recommend ship date based on carrier + route rather than shipment)

- Consolidate and standardize naming for SmartRate functions
- Add unit tests, record cassettes as needed
@nwithan8
Update CHANGELOG
7e7ff06
@nwithan8
- Linting
8c13f1e
@nwithan8
- Fix doc strings
7c90e50
@nwithan8
- Rename "ForRoute" objects to "ForZipPair"
445cbe4 
- Keep shipment-related smart rate functions in Shipment service
- Deprecate existing "estimate delivery date" functions in Shipment service
- Adjust unit test locations accordingly, re-record cassettes
@nwithan8
- Add cassettes
f1cd17f
@nwithan8
- Linting
a3234cd
@nwithan8
- Fix bad naming
f1f4c05
@nwithan8 nwithan8 force-pushed the precision_shipping branch from 53b4269 to f1f4c05 Compare July 15, 2024 20:43
@nwithan8
- Address feedback
472f196 
- Handle deprecated classes as needed, mitigations (non-breaking)
- Update tests, cassettes as needed
jchen293
jchen293 reviewed Jul 16, 2024
View reviewed changes
@nwithan8 nwithan8 requested a review from jchen293 July 16, 2024 21:48
@nwithan8 nwithan8 merged commit 38044b7 into master Jul 16, 2024
14 checks passed
@nwithan8 nwithan8 deleted the precision_shipping branch July 16, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchen293 jchen293 jchen293 approved these changes

@Justintime50 Justintime50 Awaiting requested review from Justintime50

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nwithan8 @Justintime50 @jchen293