Skip to content

Added the ability to navigate multiple SIWE notifications#18103

Merged
digiwand merged 23 commits intodevelopfrom
navigate-multiple-siwe-notifications
Jun 8, 2023
Merged

Added the ability to navigate multiple SIWE notifications#18103
digiwand merged 23 commits intodevelopfrom
navigate-multiple-siwe-notifications

Conversation

@aleksandar-mihajlovic
Copy link
Copy Markdown
Contributor

Explanation

Added the ability to navigate multiple SIWE notifications, now when user sign's multiple SIWE, the navigation header is shown so that he can go through multiple SIWE and also user has option to RejectAll.

Screenshot

Before

multipleSIWENotificationBefore

After

multipleSIWENotificationAfter

Unit tests

Before

Screenshot 2023-03-10 at 15 35 01

After

Screenshot 2023-03-10 at 16 17 55

Lint

Screenshot 2023-03-10 at 16 26 14

Manual Testing Steps

  1. Go to Test Dapp https://metamask.github.io/test-dapp/
  2. Connect MetaMask
  3. Click to Sign In With Ethereum multiple times
  4. In MetaMask extension navigation header is shown and can go through multiple notifications
  5. At the bottom of extension Reject requests button is shown
  6. When clicked there is pop up modal to Reject
  7. After reject extension is closed and there are no requests

@github-actions
Copy link
Copy Markdown
Contributor

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.

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [f5dbbbb]
Page Load Metrics (1693 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint104143123126
domContentLoaded14872076166112259
load14872172169314670
domInteractive14872076166112259
Bundle size diffs
  • background: 0 bytes
  • ui: 1606 bytes
  • common: 0 bytes

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2023

Codecov Report

Merging #18103 (ebbf314) into develop (789779f) will increase coverage by 0.00%.
The diff coverage is 77.78%.

@@           Coverage Diff            @@
##           develop   #18103   +/-   ##
========================================
  Coverage    70.04%   70.04%           
========================================
  Files          962      962           
  Lines        36875    36893   +18     
  Branches      9468     9470    +2     
========================================
+ Hits         25826    25840   +14     
- Misses       11049    11053    +4     
Impacted Files Coverage Δ
...p/signature-request-siwe/signature-request-siwe.js 68.85% <77.78%> (+3.74%) ⬆️

@dzfjz
Copy link
Copy Markdown
Contributor

dzfjz commented Mar 13, 2023

Verified by QA

@aleksandar-mihajlovic aleksandar-mihajlovic marked this pull request as ready for review March 13, 2023 09:53
@aleksandar-mihajlovic aleksandar-mihajlovic requested a review from a team as a code owner March 13, 2023 09:53
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [e14a7ee]
Page Load Metrics (1737 ± 80 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint991841272211
domContentLoaded14642106170816881
load15212106173716680
domInteractive14642106170816881
Bundle size diffs
  • background: 0 bytes
  • ui: 1606 bytes
  • common: 0 bytes

@bschorchit
Copy link
Copy Markdown
Contributor

Does this need changes so it passes codecov check?

@aleksandar-mihajlovic
Copy link
Copy Markdown
Contributor Author

Does this need changes so it passes codecov check?

I am not sure what should i do here, since this component doesnt have unit test file

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [14287f0]
Page Load Metrics (1638 ± 70 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint903301305024
domContentLoaded13521822160613063
load14531935163814570
domInteractive13511822160613063
Bundle size diffs
  • background: 0 bytes
  • ui: 1606 bytes
  • common: 0 bytes

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [dda3a40]
Page Load Metrics (1634 ± 54 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint99158120178
domContentLoaded13631811161410550
load13631856163411254
domInteractive13631811161410550
Bundle size diffs
  • background: 0 bytes
  • ui: 1606 bytes
  • common: 0 bytes

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [edb9eef]
Page Load Metrics (1651 ± 103 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1003091435828
domContentLoaded141022271649215103
load142722281651214103
domInteractive141022271649215103
Bundle size diffs
  • background: 0 bytes
  • ui: 1606 bytes
  • common: 0 bytes

aleksandar-mihajlovic and others added 3 commits March 23, 2023 16:19
# Conflicts:
#	ui/components/app/signature-request-siwe/signature-request-siwe.js
#	ui/components/app/signature-request-siwe/signature-request-siwe.test.js
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [676c2c4]
Page Load Metrics (1608 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint9412010684
domContentLoaded14231866159110952
load14231976160812259
domInteractive14231866159110952
Bundle size diffs
  • background: 0 bytes
  • ui: 1606 bytes
  • common: 0 bytes

@legobeat
Copy link
Copy Markdown
Contributor

legobeat commented Apr 10, 2023

@aleksandar-mihajlovic It looks both variations of messagesCount > 1 are not covered in tests? That is, as-is it's not testing the case with more than one message. If you add one or more cases covering this, it should pass.

Copy link
Copy Markdown
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

re: @legobeat's comment. I agree that adding a test when messagesCount < 1 would be a good addition

edit:

Thanks @aleksandar-mihajlovic for creating this PR and adding the feature support! As discussed with the team, I can get this PR merged. Feel free to reach out to me for anything regarding the PR

Comment thread ui/components/app/signature-request-siwe/signature-request-siwe.test.js Outdated
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [7088abe]
Page Load Metrics (1495 ± 26 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint8712510194
domContentLoaded1392159014755627
load1429161614955426
domInteractive1391159014755627
Bundle size diffs
  • background: 0 bytes
  • ui: 1606 bytes
  • common: 0 bytes

@digiwand digiwand self-requested a review May 30, 2023 09:40
@digiwand digiwand added the team-confirmations-secure-ux-PR PRs from the confirmations team label May 30, 2023
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [ef3eba0]
Page Load Metrics (1478 ± 20 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint88130103105
domContentLoaded1408155214713718
load1408156214784220
domInteractive1408155214713718
Bundle size diffs
  • background: 0 bytes
  • ui: 1570 bytes
  • common: 0 bytes

@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [27048fc]
Page Load Metrics (1792 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1031831282010
domContentLoaded15562103177413866
load15562103179214871
domInteractive15562103177413866
Bundle size diffs
  • background: 0 bytes
  • ui: 1572 bytes
  • common: 0 bytes

Copy link
Copy Markdown
Contributor

@NiranjanaBinoy NiranjanaBinoy left a comment

Choose a reason for hiding this comment

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

LGTM

@digiwand digiwand added DO-NOT-MERGE Pull requests that should not be merged needs-qa Label will automate into QA workspace labels Jun 7, 2023
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [ebbf314]
Page Load Metrics (1920 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1043571485024
domContentLoaded16562110188812058
load16562110192012359
domInteractive16562110188812058
Bundle size diffs
  • background: 0 bytes
  • ui: 1572 bytes
  • common: 0 bytes

@seaona
Copy link
Copy Markdown
Member

seaona commented Jun 8, 2023

hi @aleksandar-mihajlovic @digiwand PR looks good from QA side. Great job 🔥

@seaona seaona added QA Passed and removed DO-NOT-MERGE Pull requests that should not be merged needs-qa Label will automate into QA workspace labels Jun 8, 2023
@digiwand digiwand merged commit f3147bc into develop Jun 8, 2023
@digiwand digiwand deleted the navigate-multiple-siwe-notifications branch June 8, 2023 11:02
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed team-confirmations-secure-ux-PR PRs from the confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Add the ability to navigate multiple SIWE notifications

10 participants