🎟️ 🔔 📱 Custom Events + pushNotify + storeDeviceSettings + remoteNotify rewrites #1093
Conversation
React doesn't quite support custom events, so we're writing our own event handling code this will apply to our custom events, rather than the built-in events like 'onClick'
in preparation for full de-angularization, renaming the file to preserve some of the blame history
updating to React, including the event subscription model usage
for now, so they get picked up both ways
|
The custom events fit well into this paradigm, I've added the subscription calls to the "init" function that is replacing the |
|
One solution I found was to call the initialization logic using I'm not sure if this is the best place for this, but it does seem to be working |
after merging, needed to fix the differences in the was startprefs is used
the way to access the data from the event is with event.detail, not event.detail.data I discovered and fixed this, as well as adding mocks, when I started a draft test currently runs the code, but does not check that it runs correctly - used to know mocks were sufficient
now all but one feature has a passing test, working on testing the reconsent situation
moving to event-driven paradigm, so we don't call the method from pushNotify directly. this will apply to storedevicesettings once it gets migrated
after working on the consent event and process for checking for reconsent, we now have a test for that module that passes
added in test for the cloud event when subscribed and when not subscribed. additional mocking was required to handle the silent push functions
I had originally omitted the timeout because it seemed to break the test, but upon having the tests fully working, I was able to restore it this change makes the test code a little more realistic
|
This is going to have to be merged after #1072, because I pulled those changes into this PR, but the events are implemented and tested, and |
|
Ok, that's my test that's failing now, I can sometimes replicate the fail locally, let me see if I can find the cause. update: no clue how that was a "sometimes" fail (locally, it worked sometimes), since I had misspelled a key in my mock config, but I've resolve that and ironed out some of the "timeouts" (the events set of actions, which take some time to complete, but are async by design) and I centralized the |
moved the clear plugin into an "afterEach" call to clean up the code corrected a key spelling that fixed one of the tests, and updated the way I wait for the event handling to happen https://stackoverflow.com/questions/45478730/jest-react-testing-check-state-after-delay
syncing up the ways that tests run, replacing the way timeouts are set, removing unneeded timeout extension
I found two problems to fix in my testing - first was that I was not properly removing the event listeners and learned more: https://macarthur.me/posts/options-for-removing-event-listeners Second was that I was missing a mock, which meant some of the storage operations were failing due to that function not exisiting
now testing for settings stored on initialization, on consent, and on intro done also testing for not done if not consented on initialization, and if intro not done on consent
Abby-Wheelis
changed the title
|
On checking for anything else listening for the events, I found that Since these changes alter the way we handle events, I'm probably going to move ahead with rewriting that file here as well. That will make this PR relatively big, but should take care of all the event-driven functionality in splash at once. |
this is leftover code and I never used it in testing, removing
hoping to preserve some commit history here remotenotify uses the cloud notif event, so rewritting alongside the rest of the event-driven code
migrate the file into typescript, including adding the event handling remove references (all unused) add init call with others in App.tsx
prettier wanted to make a bunch of changes to code that I didn't edit, so putting it in a dedicated commit
since our eventual goal is to remove angular, removing these calls since it will evaluate false if undefined anyways
checks for no action if not subscribed and action if subscribed
adding more tests - leveraged mocks to ensure that calls to open url or create popup were happening at the appropriate times
|
This PR now encompasses all of the uses of the events for "intro done" "consent done" and "cloud notification". This includes |
Abby-Wheelis
changed the title
There was a problem hiding this comment.
This is really cool and not nearly as complex as I thought!
| data.additionalData.payload && | ||
| data.additionalData.payload.alert_type | ||
| ) { | ||
| if (data.additionalData.payload.alert_type == 'website') { |
There was a problem hiding this comment.
@shankari Are we ever going to re-implement external web launches via push notification? If not, we can simplify
There was a problem hiding this comment.
I think we should re-implement all the different types of push notification. I have pitched the external web launches, in particular, as mechanism for providing incentives for deployments with autogenerated tokens. The program admin can send a link to an amazon gift card directly to the participant though a push notification, without having to know their email/phone number etc.
Follow on to e-mission/e-mission-docs#976
we still have the information through debug statements
e-mission#1093 (comment) kept to remain consistent within file
when I moved from the formatted alert to a plain alert, the text that ends up in the alert changed, but I did not update my test also added a space between the title and the text
- borrowed the windowAlert mocks from PR e-mission#1093, to allow error checking - added mockCheck, updated some tests to use to `toEqual()`
|
This has a merge conflict. Let me try to resolve if it is easy to avoid getting stuck in a circular loop... |
| markIntroDone(); | ||
| await new Promise((r) => setTimeout(r, 500)); | ||
| publish(EVENTS.CONSENTED_EVENT, 'test data'); |
There was a problem hiding this comment.
future fix: I think you also want to check that the user settings are not set before the CONSENTED_EVENT even though the intro is done.
|
@JGreenlee merging this now - another potential source of merge conflicts |
This will replace #1090, after we re-evaluated and made a decision to continue forward with custom events in #1024. I elected to start a fresh PR to keep the blame as clean as I could.
This introduces custom events, and migrates
pushNotifyto typescript. As this PR stands, there will be events "duplicated" because some applications still use$emitand$onwhile others use the newpublishandsubscribe. Where applicable, some events are broadcast using both methods, so they can be received by both methods.