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

Removing support survey notification from What's New #11118

Merged
merged 3 commits into from
May 18, 2021
Merged

Removing support survey notification from What's New #11118

merged 3 commits into from
May 18, 2021

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented May 17, 2021

Test Plan

  1. Ensure from a fresh install that the support notification is not shown
  2. Ensure the support notification is removed after an upgrade where it would otherwise be shown

@ryanml ryanml requested a review from danjm May 17, 2021 20:26
@ryanml ryanml self-assigned this May 17, 2021
@ryanml ryanml requested a review from a team as a code owner May 17, 2021 20:26
ryanml
ryanml commented May 17, 2021
View reviewed changes
Copy link
Contributor Author

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

I didn't re-index the translation keys as I didn't want to cause an issue with translations already in progress

@ryanml ryanml changed the title Removing support notification from What's New Removing support survey notification from What's New May 17, 2021
Gudahtt
Gudahtt reviewed May 17, 2021
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

If we're going to leave the translation keys as-is, perhaps it would be simpler to leave the notification IDs as-is, and remove #2? Rather than shift them all by one. There is no requirement for them to be consecutive.

Also, we should remove the translations that we're no longer using.

@ryanml ryanml force-pushed the remove-support-notification branch from 931679c to f33a457 Compare May 17, 2021 22:14
@ryanml ryanml requested a review from Gudahtt May 17, 2021 22:14
@ryanml
Copy link
Contributor Author

ryanml commented May 17, 2021

No longer re-indexing notification ids/keys. Good thought to keep it simple. Also, removed the now unused translations

@metamaskbot
Copy link
Collaborator

Builds ready [f33a457]
Page Load Metrics (612 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint448463105
domContentLoaded42280061010249
load42380161210249
domInteractive42280061010249

Gudahtt
Gudahtt reviewed May 18, 2021
View reviewed changes
app/scripts/migrations/060.js Outdated

function transformState(state) {
if (
state?.NotificationController?.notifications[SUPPORT_NOTIFICATION_KEY]
Copy link
Member

Choose a reason for hiding this comment

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

Nit: it would be great to have tests for all branches, such as the case where NotificationController is nullish.

Probably best to add a few more ?. operators here as well, in case notifications is nullish, and in case notifications[SUPPORT_NOTIFICATION_KEY] is nullish. So that this won't crash if the state is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

transformState updated to accommodate instances where NotificationController or notifications are non-existent. Test cases added to cover additional branches

Gudahtt
Gudahtt previously approved these changes May 18, 2021
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM! Left a few suggestions on making tests more robust.

danjm
danjm previously approved these changes May 18, 2021
View reviewed changes
@ryanml ryanml dismissed stale reviews from danjm and Gudahtt via 558eb6a May 18, 2021 04:35
@ryanml ryanml requested review from Gudahtt and danjm May 18, 2021 04:35
@ryanml ryanml force-pushed the remove-support-notification branch from 558eb6a to c188bdb Compare May 18, 2021 04:41
@metamaskbot
Copy link
Collaborator

Builds ready [c188bdb]
Page Load Metrics (623 ± 45 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint489862115
domContentLoaded4058156229345
load4068186239345
domInteractive4048156229345

@ryanml ryanml merged commit 86b61a2 into develop May 18, 2021
@ryanml ryanml deleted the remove-support-notification branch May 18, 2021 17:55
Gudahtt added a commit that referenced this pull request May 21, 2021
@Gudahtt
Merge remote-tracking branch 'origin/develop' into sync-master
203456d 
* origin/develop: (227 commits)
  Improve UI + content for price difference notifications (#11145)
  Swaps: Create a new swap (#11124)
  Bump @metamask/controllers from 9.0.0 to 9.1.0 (#11150)
  Capture exception instead of throw error in useTransactionDisplayData (#11153)
  Fixing jest component test output errors (#11139)
  Avoid showing  "Gas price extremely low" warning in advanced tab for testnets (#11111)
  @metamask/[email protected] (#11140)
  Migrate to new CurrencyRateController (#11005)
  bump allow scripts (#11134)
  Show Sentry CLI output when uploading artifacts (#11100)
  use etherscan-link customBlockExplorer methods with customNetwork usage tracking (#11017)
  Adding notification for updated seed phrase wording (#11131)
  Bumping package.json
  Fix a condition for checking if a token should be added (#11127)
  Removing support survey notification from What's New (#11118)
  Handling custom token decimal fetch failure due to network error (#10956)
  Hide basic tab in advanced gas modal for speedup and cancel when on testnets (#11115)
  Migrate Sentry settings to environment variables (#11085)
  Update eth-ledger-bridge-keyring to v0.5.0 (#11064)
  fix metaRPCClientFactory id handling (#11116)
  ...
danjm pushed a commit that referenced this pull request Jun 6, 2021
@ryanml @danjm
Removing support survey notification from What's New (#11118)
b8b62a1 
* Removing support notification from what's new

* Adding migration for support notification removal

* Expanding test cases, using async/await for storage comparison
danjm pushed a commit that referenced this pull request Jun 7, 2021
@ryanml @danjm
Removing support survey notification from What's New (#11118)
c9a539b 
* Removing support notification from what's new

* Adding migration for support notification removal

* Expanding test cases, using async/await for storage comparison
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danjm danjm danjm approved these changes

@Gudahtt Gudahtt Awaiting requested review from Gudahtt

Assignees

@ryanml ryanml

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ryanml @metamaskbot @Gudahtt @danjm