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

[feat] Consolidate SmartRate functions, add recommended ship date function #566

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

nwithan8
Copy link
Member

Description

  • Add new SmartRate service under beta for interacting with the SmartRate API
    • New RecommendShipDateByDeliveryDate function to recommend a ship date based on a delivery date for each shipment rate
    • Updated EstimateDeliveryDateByShipDate function to get an estimated delivery date based on a ship date for each shipment rate
    • Existing SmartRate-related functions and classes in Shipment service marked as deprecated and will be removed in a future release

The entire service is under beta, although inside it the EstimatedDeliveryDateByShipDate and GetSmartRates functions in it point to GA endpoints. Only RecommendShipDateByDeliveryDate is a new "beta" endpoint.

When the service is promoted to GA, the existing SmartRate functions in the Shipment service should be removed.

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)

… 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 nwithan8 requested a review from a team as a code owner May 23, 2024 22:37
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.

EasyPost.Tests/ServicesTests/Beta/SmartRateServiceTest.cs Outdated Show resolved Hide resolved
EasyPost.Tests/ServicesTests/Beta/SmartRateServiceTest.cs Outdated Show resolved Hide resolved
EasyPost/Models/API/RateWithEstimatedDeliveryDate.cs Outdated Show resolved Hide resolved
EasyPost/Models/API/TimeInTransitDetails.cs Outdated Show resolved Hide resolved
Copy link
Member

@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

/// Class representing a set of SmartRate-related beta functionality.
/// </summary>
// ReSharper disable once ClassNeverInstantiated.Global
public class SmartRateService : EasyPostService
Copy link
Member

Choose a reason for hiding this comment

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

Should this class be out of beta when the endpoints are in GA?


/// <summary>
/// Retrieve the estimated delivery date of each rate for a <see cref="Shipment"/> via the SmartRates API, based on a specific ship date.
/// <a href="https://www.easypost.com/docs/api#retrieve-estimated-delivery-date-and-total-transit-days-across-all-rates-for-a-shipment">Related API documentation</a>.
Copy link
Member

Choose a reason for hiding this comment

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

URL seems incorrect to what this function does, you might want to add a TODO and come back to fix it when API docs is added for this endpoint

@Justintime50 Justintime50 marked this pull request as draft June 13, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants