Fix Bug 3431: Clean up usage of ActivityStreamPrefs to use Prefs from the store #3917

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@SeanPrashad
Contributor

SeanPrashad commented Jan 4, 2018

Fixes #3431: Update redundant .get() calls to ActivityStreamPrefs to use this.store.getState().Prefs.values. Also updating the respective unit tests to accomodate this new behaviour.

Note: Currently a WIP as of Jan 4, 2018

@@ -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": {}});

This comment has been minimized.

@Mardak

Mardak Jan 5, 2018

Member

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.

@Mardak

Mardak Jan 5, 2018

Member

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
+ }
+
+ set setMigrationRemainingDays(newDate) {
+ this.store.getState().Prefs.values.migrationRemainingDays = newDate;

This comment has been minimized.

@Mardak

Mardak Jan 5, 2018

Member

We don't want to directly modify the store without notifying others. See expireMigration in this file.

@Mardak

Mardak Jan 5, 2018

Member

We don't want to directly modify the store without notifying others. See expireMigration in this file.

system-addon/lib/ManualMigration.jsm
+ }
+
+ get getMigrationRemainingDays() {
+ return this.store.getState().Prefs.values.migrationRemainingDays;

This comment has been minimized.

@Mardak

Mardak Jan 5, 2018

Member

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?

@Mardak

Mardak Jan 5, 2018

Member

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?

This comment has been minimized.

@k88hudson

k88hudson Jan 29, 2018

Member

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.

@k88hudson

k88hudson Jan 29, 2018

Member

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.

@SeanPrashad

This comment has been minimized.

Show comment
Hide comment
@SeanPrashad

SeanPrashad Jan 13, 2018

Contributor

@Mardak I've updated the ManualMigrations import statement as requested and modified both setters to update the store as appropriate.

I'm now attempting to switch over the old fakePrefs statements in ManualMigration.test.js but I'm unsure what the old fakePrefs.get.returns(today); call would look like in the updated version - any suggestions?

Contributor

SeanPrashad commented Jan 13, 2018

@Mardak I've updated the ManualMigrations import statement as requested and modified both setters to update the store as appropriate.

I'm now attempting to switch over the old fakePrefs statements in ManualMigration.test.js but I'm unsure what the old fakePrefs.get.returns(today); call would look like in the updated version - any suggestions?

@SeanPrashad SeanPrashad changed the title from Clean up usage of ActivityStreamPrefs to use Prefs from the store to Fix Bug 3431: Clean up usage of ActivityStreamPrefs to use Prefs from the store Jan 19, 2018

@k88hudson

This comment has been minimized.

Show comment
Hide comment
@k88hudson

k88hudson Jan 29, 2018

Member

@SeanPrashad This is looking great so far! For the tests, instead of using the fakePrefs helper, you'll want to set whatever value you need on your instance.store.state object. For example, instead of fakePrefs.get.returns(today), you would set instance.store.state.migrationLastShownDate = today. Does that make sense?

Member

k88hudson commented Jan 29, 2018

@SeanPrashad This is looking great so far! For the tests, instead of using the fakePrefs helper, you'll want to set whatever value you need on your instance.store.state object. For example, instead of fakePrefs.get.returns(today), you would set instance.store.state.migrationLastShownDate = today. Does that make sense?

@k88hudson k88hudson self-requested a review Jan 29, 2018

@k88hudson k88hudson self-assigned this Jan 29, 2018

@SeanPrashad

This comment has been minimized.

Show comment
Hide comment
@SeanPrashad

SeanPrashad Jan 30, 2018

Contributor

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!

Contributor

SeanPrashad commented Jan 30, 2018

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!

@Mardak

This comment has been minimized.

Show comment
Hide comment
@Mardak

Mardak Mar 29, 2018

Member

Thanks for putting together this initial PR! Superseded by #4071.

Member

Mardak commented Mar 29, 2018

Thanks for putting together this initial PR! Superseded by #4071.

@Mardak Mardak closed this Mar 29, 2018

@k88hudson

This comment has been minimized.

Show comment
Hide comment
@k88hudson

k88hudson Mar 29, 2018

Member

Thanks for your work on this! Your patch landed in this commit b634b4b with some additional test fixes here: (2c8e242)

Member

k88hudson commented Mar 29, 2018

Thanks for your work on this! Your patch landed in this commit b634b4b with some additional test fixes here: (2c8e242)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment