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

Fix CarrierAccount creation endpoint for UpsAccount and FedexAccount #229

Merged
merged 8 commits into from
Dec 6, 2022

Conversation

aryrabelo
Copy link
Contributor

Description

This fixes CarrierAccount creation endpoint for UpsAccount and FedexAccount that according to the docs https://support.easypost.com/hc/en-us/articles/360041148012-FedEx-Account-Registration-Guide and https://support.easypost.com/hc/en-us/articles/360024355712-Setting-up-your-UPS-Account should be sent to a different endpoint.

Testing

Added tests

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)

@aryrabelo aryrabelo requested a review from a team as a code owner November 10, 2022 16:24
@Justintime50
Copy link
Member

Hey @aryrabelo, thanks for your PR! We are making this change across our other libs too, funny timing you submitted this now.

There is a bit of nuance with the /register endpoint that we are working through internally right now (we have two types of endpoints for UPS/FedEx and are determining which this should really go to so we can get these in the libs and the docs correctly.) We'll make sure to update you once we've determined how to move forward over the next couple days.

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.

This is pretty close. You can look at #231 for an example of what I'm thinking of and/or look at our Python/C#/Go libraries recent PRs where we just added this too.

If you can address these couple items of feedback, we can get this merged in and released. Thanks!

lib/easypost/carrier_account.rb Outdated Show resolved Hide resolved
spec/carrier_account_spec.rb Outdated Show resolved Hide resolved
spec/carrier_account_spec.rb Outdated Show resolved Hide resolved
spec/carrier_account_spec.rb Outdated Show resolved Hide resolved
@Justintime50
Copy link
Member

@aryrabelo are you able to make the adjustments suggested here? We'd like to get this in a release next week. We can make the adjustments next week if you aren't able. Thanks!

@aryrabelo
Copy link
Contributor Author

@Justintime50 Sorry, I took a week off, pushing the changes now

@Justintime50
Copy link
Member

No worries! The code looks good; however, Ruby isn't getting setup correctly on CI. I pushed a fix for this problem last week that is on the master branch. If you can rebase this PR, we should be golden and we'll get this merged in.

@Justintime50
Copy link
Member

It appears that GitHub Actions is failing due to a new issue for psych which just had v5 released yesterday (not related to my fix for something else last week). I'm investigating what's needed to fix this and will report back shortly.

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.

Thanks again for this PR! Can't wait to get your contribution into the project.

@Justintime50 Justintime50 merged commit 4c58b9a into EasyPost:master Dec 6, 2022
@Justintime50
Copy link
Member

For future travelers, the CI build was failing due to ruby/psych#597. For now, I've pinned the previous version until this gets fixed for v5 of psych and can build successfully on Ubuntu (what we use on GitHub Actions)

@Justintime50
Copy link
Member

Your changes have been release in v4.9.0, thanks again for your contribution!

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