Rewrite StartPrefs #1072
Rewrite StartPrefs #1072
Conversation
first attempt at converting to a ts file, not fully functional, but did find several functions that were no longer used and so have been removed
we are now not using the angular service anymore, so should access everything from the new ts file
we had a bug related to this call, so adding a note to future development to make sure we're aware of the ramifications that this can cause.
was breaking because I spelled cordova wrong
|
The tests are currently failing because of a timeout issue with the React test (for LoadMoreButton) sometimes locally, and it looks like that's the case here as well. There is a way to expand the time limit, which I've done for one or two other tests. |
|
One small problem that I'm running into here is that one of the functions relies on |
Looking through the code base here, I can't really find where this event is received. What's emitted is "data_collection_consented" which I have been unable to find outside of this file on either Ah! But when I look for |
|
I was just able to talk through this a bit with @sebastianbarry, and what we settled on was extracting the functionality from the I haven't quite implemented this yet, but it seems like a good migration strategy. |
remove unused key, update to single source of truth for INTRO_DONE_KEY
Pre-migration, there was a "consent done" event broadcasted from startprefs whenever the user marked consent. However, Reach and typescript does not handle events like this. Instead, we are extracting the code into a function, retrieving it from the plugin (for now), and then calling those functions when consent is marked. As a part of this, unify the reading of "isIntroDone" to the method in onboardingHelper, rather than storing a variable in startPrefs
we actually need to wait until fully consented, and the user registered before we can take the actions in the plugins I noticed this because of an error message that appeared when logging in, indicating that the user was not registered, so the device settings could not be stored moving these calls to after the user has FULLY consented, resolved that issue
adding descriptions to the functions, and removed some unused imports
|
This approach seems to be working based on logging in and out of the emulator and watching the log statements. The next step will be to write tests. This will not be very direct, since the file involves a lot of reading and writing from storage, but hopefully I can leverage existing mocks to help with that. |
after adding more mocked functions, the startprefs now have one test, for getting the consent doc from storage
this test needed a mock of markConsented in the BEMDataCollection plugin, I have added _storage so that when the document is written in, it can also be retrieved -- even though that happens in two different plugins
since testing separately affects the local storage, testing in sequence allows for easier testing. return json from fetch rather than string to make it parseable in storage, had to check for the string undefined, since that was returned in one of the tests
…into startprefs-rewrite also resolved merge conflicts
for consistency, since this is also a function that is async
since we no longer work with the IRB protocol within the app, we don't need to present it to the user
|
After resolving merge conflicts and removing some of the old process, now I'm waiting until we make a final decision on how to handle the "check consent" from profile settings this comment before I think this is ready for merge. |
|
As decided in #1016:
This is currently implemented:
|
www/js/splash/storedevicesettings.js
Outdated
| $rootScope.$on(startPrefs.INTRO_DONE_EVENT, function(event, data) { | ||
| console.log("intro is done -> original consent situation, we should have a token by now -> register"); | ||
| storedevicesettings.storeDeviceSettings(); | ||
| }); |
There was a problem hiding this comment.
It looks like INTRO_DONE_EVENT is no longer emitted from anywhere. So I don't think you need to listen for it anymore, but I do think you need to find a suitable replacement.
From analyzing these 2 events (CONSENTED_EVENT and INTRO_DONE_EVENT) and seeing how the old StartPrefs worked, here's the high-level takeaway I'm getting.
When either:
- a) the intro gets done
- b) the user reconsents (consent gets done and intro was already done)
We call storedevicesettings.storeDeviceSettings() and pushnotify.registerPush.
So I think the afterConsent mechanism you made should be something more like afterIntroOrReconsent, and I think SaveQrPage is the wrong place to do it.
I'm not sure if reconsent is still a thing or will be a thing in the future (I saw there was recent discussion on this). If not, I suppose you'll only need to consider (a) and not (b)
There was a problem hiding this comment.
We're taking out reconsent for now, as decided in this comment. Combining the mechanism makes sense, I'll work on combining things and finding a better place for it.
There was a problem hiding this comment.
a) the intro gets done
One place to catch this would be to add the functionality to the markIntroDone method in onboarding helper
b) the user reconsents (consent gets done and intro was already done)
I could add the after consent part to markConsented in startPrefs - (if Intro is done) this would be a very similar place functionally, but maybe better organizationally?
@JGreenlee do those places sound better?
There was a problem hiding this comment.
Yes, those 2 places seem appropriate to me
There was a problem hiding this comment.
Done! I tested by logging in and out of the emulator and seeing the log statement for the "after intro events" and not the log statement for the "reconsent events". This is expected behavior since reconsent is not what's happening on login, and we don't currently have a reconsent scenario. I'll add some git comments to the new code to flag the changes.
saveQR was a bad place to handle this, moving to markConsented As Jack pointed out, on a reconsent (mark consent after intro done) we want to call registerPush and storeDeviceSettings - that is what is done here centralizing the logic to markConsented eliminated the need for the functions in the other files, since we check for consent done just once
We no longer emit this event, but still need to handle the logic On marking intro done, we will registerPush and storeDeviceSettings e-mission#1072 (comment)
There were several places where I had other log statements, but logDebug was more appropriate, see review for details e-mission#1072 (comment)
Abby-Wheelis
changed the base branch from
service_rewrite_2023
to
onboarding_routing_sept_2023
Abby-Wheelis
changed the base branch from
onboarding_routing_sept_2023
to
service_rewrite_2023
This PR will rewrite the service in
splash/startprefs.js, as noted in this comment. It might also be time to re-consider exactly how consent is handled, as discussed in this issue. After the patch in #1071, I anticipate some merge conflicts, since the files overlap, but since I've worked on both changes I'm hopeful to be able to resolve them quickly.