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

Deprecate snake case from feature flags #5031

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

nikoferro
Copy link
Member

Description

This change deprecates the use of snake case keys on feature flags on swaps.
For this the swaps-api has been updated to return both snake case and camel case values.

Once this PR and a similar one on the extension are merged, snake case values can be safely removed from swaps api

Screenshots/Recordings

Screenshot 2022-09-27 at 13 18 41

Issue

See MMS-255

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@nikoferro nikoferro requested a review from a team as a code owner September 27, 2022 11:25
@github-actions
Copy link
Contributor

github-actions bot commented Sep 27, 2022

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@nikoferro
Copy link
Member Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

LGTM

@gantunesr
Copy link
Member

Hi @nikoferro, does this PR require feature QA?

@nikoferro
Copy link
Member Author

Hi @nikoferro, does this PR require feature QA?

Hi @gantunesr , IMHO there isn't any special QA for this other than to check that swaps still get the "enabled" flag for mobile (which i have checked on my end).

The camel case key that enables swaps, was already present on the feature flags endpoint response, so this should be a harmless change

@gantunesr gantunesr added the Spot Check on the Release Build If a ticket doesn't require feature QA, but does require some form of manual spot checking label Sep 27, 2022
@gantunesr
Copy link
Member

Sounds good @nikoferro. Can you comment again "I have read the CLA Document and I hereby sign the CLA"? Not sure why it's failing. After that I can merge the PR

@nikoferro
Copy link
Member Author

I have read the CLA Document and I hereby sign the CLA

@nikoferro
Copy link
Member Author

@gantunesr Ha! there it goes, i thought you needed to trigger this manually after the first failed check

@gantunesr
Copy link
Member

I did trigger it manually but it's not working for some reason I haven't looked into it yet haha but commenting for a 2nd time is a easy workaround

@gantunesr
Copy link
Member

During regression it's only necessary to verify that Swaps is still functional for the mobile app, is not necessary any other extra test case besides the regular ones @cortisiko @chrisleewilcox

@gantunesr gantunesr merged commit 5730b0c into MetaMask:main Sep 27, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Sep 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-5.9.0 Spot Check on the Release Build If a ticket doesn't require feature QA, but does require some form of manual spot checking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants