Fix Bug 3431: Clean up usage of ActivityStreamPrefs to use Prefs from the store #3917
Conversation
| @@ -38,17 +42,23 @@ describe("ManualMigration", () => { | |||
| } | |||
| }; | |||
|
|
|||
| const {ManualMigration} = injector({"lib/ActivityStreamPrefs.jsm": {Prefs: fakePrefs}}); | |||
| // const {ManualMigration} = injector({"lib/ActivityStreamPrefs.jsm": {Prefs: fakePrefs}}); | |||
| const {ManualMigration} = injector({"lib/ActivityStreamPrefs.jsm": {}}); | |||
There was a problem hiding this comment.
The injector was only needed to pass in a fake prefs, so because we're no longer using that, we should be able to just import ManualMigration as a regular module at the top of the test file.
system-addon/lib/ManualMigration.jsm
Outdated
| } | ||
|
|
||
| set setMigrationRemainingDays(newDate) { | ||
| this.store.getState().Prefs.values.migrationRemainingDays = newDate; |
There was a problem hiding this comment.
We don't want to directly modify the store without notifying others. See expireMigration in this file.
system-addon/lib/ManualMigration.jsm
Outdated
| } | ||
|
|
||
| get getMigrationRemainingDays() { | ||
| return this.store.getState().Prefs.values.migrationRemainingDays; |
There was a problem hiding this comment.
Thanks for cleaning this up. I think instead of having individual getter/setters for each pref, it would be simple enough to have a helper that takes in the pref name. Although at that point, it's nothing specific to manual migration, so maybe it should be available for all Feeds?
@k88hudson any thoughts on having this.store.getPref(name) and similar setPref helper that dispatches?
There was a problem hiding this comment.
I can see that being helpful, yeah; I guess it is a bit verbose to do this.store.getState().Prefs.values every time.
I think either extending store to have store.getPref and store.setPref as helpers or injecting another helper (such as this.prefs, at the same level as store) would be ok, but if it's out of the scope of this PR a single getter/setter in ManualMigration.jsmseems fine to me too.
|
@Mardak I've updated the ManualMigrations I'm now attempting to switch over the old |
seanprashad
changed the title
|
@seanprashad This is looking great so far! For the tests, instead of using the |
|
Thank you @k88hudson! That makes much more sense - I will try to take a crack at it sometime soon! Edit: Help getting this across the finish line from anyone is welcomed with open arms! |
|
Thanks for putting together this initial PR! Superseded by #4071. |
Fixes #3431: Update redundant
.get()calls to ActivityStreamPrefs to usethis.store.getState().Prefs.values. Also updating the respective unit tests to accomodate this new behaviour.Note: Currently a WIP as of Jan 4, 2018