Skip to content

Conversation

@nwithan8
Copy link
Contributor

@nwithan8 nwithan8 commented Aug 3, 2022

Description

We noticed that, despite it being deprecated, the old getSmartrates function is still being called when updating a shipment, as our merge function believes it is a getter. This results in erroneous API calls.

The old function is slated for removal "eventually", but until then, we need to skip the function.

Rather than hard-coding a skip for this specific function, we have added an if clause to the merge functionality that checks if a function is marked as deprecated. If it is, the function is skipped.

Testing

  • All unit tests pass as expected
  • Re-recorded two cassettes that had the bad API call in them. Confirmed that bad API call is no longer being made after fix.

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 August 3, 2022 17:09
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.

Question: Why skip deprecated methods altogether? Couldn't this have unintended side-effects vs hard-coding the known one bad call?

@nwithan8
Copy link
Contributor Author

nwithan8 commented Aug 3, 2022

Question: Why skip deprecated methods altogether? Couldn't this have unintended side-effects vs hard-coding the known one bad call?

So this doesn't happen again where we might not notice it or catch it.

If we've marked a function as deprecated, we don't want people to use it. It also means WE don't want to use it; we usually rewrite and adjust as much as possible to avoid a deprecated function when we do mark something as deprecated.

@nwithan8 nwithan8 merged commit 388cbd1 into master Aug 3, 2022
@nwithan8 nwithan8 deleted the smartrate_fix branch August 3, 2022 17:31
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.

4 participants