Paths

Table of Contentst

Differential D15276

[Cashtab] Optimize notifications - SignVerifyMsg
AbandonedPublic
Actions

Authored by emack on Jan 25 2024, 01:27.

Details

Reviewers

bytesofman

Group Reviewers

Restricted Project

Summary

The current implementation of notifications across Cashtab is a smorgasbord of 12 different antd functions that take in slightly different parameters.

This diff optimizes this by consolidating them into a single function that is not context specific, which will eventually enable the deprecation of the existing 12 notif functions.

Given the magnitude of changes across the app, this will be landed in increments per component, with one final diff deprecating all 12 existing notification functions.

The SignVerifyMsg component is the first cab off the rank, which ended up being a tricky one to start with due to modal interactions and default closure of dropdowns.

Test Plan

npm test
grep -r messageSignedNotification src/ with no output
npm start
Navigate to the Sign & Verify component and trigger the following notifications:
Signature generation success
Click the generated signature (Copy to clipboard notif)
Signature verification success
Signature verification failure
npm run build, or load notifs.netlify,.app on android and ios mobile devices and test the above
npm run extension, test the above on extension

Diff Detail

Repository

rABC Bitcoin ABC

Branch

optimizeNotifs

Lint

Lint Errors

Severity	Location	Code	Message
Error	cashtab/src/components/SignVerifyMsg/__tests__/SignVerifyMsg.test.js:8	ESLINT	no-unused-vars
Error	cashtab/src/components/SignVerifyMsg/__tests__/SignVerifyMsg.test.js:12	ESLINT	no-unused-vars
Error	cashtab/src/components/SignVerifyMsg/__tests__/SignVerifyMsg.test.js:70	ESLINT	no-unused-vars
Error	cashtab/src/components/SignVerifyMsg/__tests__/SignVerifyMsg.test.js:110	ESLINT	no-unused-vars
Error	cashtab/src/components/SignVerifyMsg/__tests__/SignVerifyMsg.test.js:155	ESLINT	no-unused-vars

Unit

No Test Coverage

Build Status

Buildable 26640
Build 52848: Build Diff	cashtab-tests
Build 52847: arc lint + arc unit

Event Timeline

emack created this revision.Jan 25 2024, 01:27

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 25 2024, 01:27

emack requested review of this revision.Jan 25 2024, 01:27

Harbormaster completed remote builds in B26640: Diff 44569.Jan 25 2024, 01:27

Removed unused declarations

Harbormaster completed remote builds in B26641: Diff 44570.Jan 25 2024, 01:36

Trying to do too much here. Should not take this many diffs to implement, provided the first diff is right.

1 - Create a notification component (or function, or hook) with unit tests for all the cases the various notification components are currently used for
2 - Drop it in in-place of all the other functions

cashtab/src/components/Common/Notifications.js
40 ↗	(On Diff #44570)	replacing 12 notifications with a function that has 5 hardcoded types is not really a big win. We want one notification component that is flexible enough to be called from any existing site and accepts params. If `type` is one of those params, that's fine, but having a switch statement that then handles each different one seems a bit much, esp when the only difference is the key on an object being called.
44 ↗	(On Diff #44570)	we need to out of the habit of naming params `optionalSomething` if it's optional, just assign the default in the function.

This revision now requires changes to proceed.Jan 25 2024, 04:04

Refactored displayNotification for efficiency and flexibility, including unit tests reflecting the existing 12 notifications.

Harbormaster completed remote builds in B26649: Diff 44580.Jan 25 2024, 13:13

bytesofman requested changes to this revision.Jan 25 2024, 14:40

bytesofman added inline comments.

cashtab/src/components/Common/Notifications.js
45 ↗	(On Diff #44580)	this should not be a param. Test if the element appears and has the right content. Not if it has the right test id, which of course it does bc the test passed it.
47 ↗	(On Diff #44580)	what happens if no icon is supplied, but duration is supplied? will duration be read as icon and result in an error? might need to make this reqd.
58 ↗	(On Diff #44580)	instead of this `or` conditional, use `link !== false &&` -- so the link is either rendered or not. Avoid repeating the <Paragraph> msg div. Also we want the data-testid on Paragraph fo the link as well.
cashtab/src/components/SignVerifyMsg/SignVerifyMsg.js
11 ↗	(On Diff #44580)	we can back out all implementation from this diff.
cashtab/src/components/SignVerifyMsg/__tests__/SignVerifyMsg.test.js
1 ↗	(On Diff #44580)	signverify msg implementation and tests should be in another diff.

This revision now requires changes to proceed.Jan 25 2024, 14:40

icon param is now required, and the callsite can supply a 'default' value if they want to use the default antd icons, otherwise can provide one from CustomIcons.js.
data-testid now removed as a param and hardcoded as 'cashtab-notification'
updated conditional rendering of hyperlink in the notification description via the new <NotificationLink> styled component. Can use false to indicate a non-hyperlinked notification.

The callsite is per below for a hyperlinked :

displayNotification(
	'success',
	title,
	msg,
	<NotificationLink link={'http://test.com'}/>,
	'default',
);

which generates the following

We can enhance the cosmetics further in a separate diff.

backed out SignVerifyMsg implementation in SignVerifyMsg.js
backed out integration tests in SignVerifyMsg.test.js
backed out implementation in CopyToClipboard.js
re-instated messageSignedNotification in Notifications.js
re-instated MessageSignedNotificationIcon in CustomIcons.js

Harbormaster completed remote builds in B26674: Diff 44618.Jan 26 2024, 13:40

There needs to be more thought overall in "how will this diff improve the app."

The starting point here was instead mimicking the existing app feature in a slightly-less-bad-but-still-bad way.

It's okay to throw something out entirely if it's garbage. We should not need to go through so many iterations for the kinds of issues that have surfaced in this diff.

Also please update the diff description and title to reflect the current state.

Best path forward from here is probably just a series of diffs that replace our weirdly implemented functions with pure calls to the antd component. We don't need integration tests that confirm the notification function shows what we asked it to show. For notifications, we want integration tests that show "the expected notification is displayed in the expected case" -- see how this is currently tested in the last test of SendXec.test.js

cashtab/src/components/Common/Notifications.js
46 ↗	(On Diff #44618)	so this is not a `string`, it can only be certain strings, e.g. `'success' \|\| 'error \|\| ...` etc
49 ↗	(On Diff #44618)	so now it's `React.Component \|\| false`
59 ↗	(On Diff #44618)	ok so now we have a param with a default, followed by a required param, followed by a param with a default you can just make these all optional
77 ↗	(On Diff #44618)	ok I don't really see any improvement with this function vs just straight-up using the antd component. all it does is add a standardized `<Paragraph>` component. OK, well we can have a standardized styled component, it doesn't have to be a function that takes the same params as this also-existing function.
cashtab/src/components/Common/__tests__/Notifications.test.js
10 ↗	(On Diff #44618)	all of these are just unit tests for antd's notification function. we don't need to test this. see the integration test for notification at the end of SendXec.test.js

This revision now requires changes to proceed.Jan 26 2024, 14:06

Following tg discussions, will be replacing each component notification callsites with pure calls to the antd component, as there is little value in retaining a dedicated Notifications.js component. This will be done in a series of diffs, one for each component.