Framework: Convert NotificationsSettings store to redux & remove Immutable from module. #28653

spen · 2018-11-18T14:50:27Z

Changes proposed in this Pull Request

This PR started as an effort to remove ImmutableJS from Calypso (#28652), replacing it's usage in the NotificationsSettings store with plain JS. Doing that took us most of the way to replacing the entire flux store with redux state - one step closer to removing Immutable (there's now only one module left) and one step closer to removing flux altogether too.

Testing instructions

Visit each of the following links (you can use the tabs in Calypso) and try toggling some settings on each (they each use the different toggleState methods).
Hit save and refresh the page to ensure that the settings have persisted.

http://calypso.localhost:3000/me/notifications
http://calypso.localhost:3000/me/notifications/comments
http://calypso.localhost:3000/me/notifications/updates

You should also notice either a success or error notice depending on the response of the save request (you can mock a failure by 'throttling' your network to mimick being offline).

Notes:

`blogs`/`blog` in `toggleState`

NotificationSettingsStore.getStateFor gets called with the values 'other', 'wpcom' and 'blogs' whereas toggleState has the methods other, wpcom and blog. This is accounted for by toggleSetting, which defaults to calling the blog method when a source is passed in that it doesn't have a method to match. I don't think that the blog method ever actually gets called directly, only via defaulting. I believe it likely that we can just rename the blog method to blogs and never have the need for defaulting - this would need investigation though, to be sure.

matticbot · 2018-11-18T14:50:29Z

Test live: https://calypso.live/?branch=update/refactor-notifications-toggle-state

jsnajdr · 2018-11-22T12:47:43Z

Hello @spen, thanks for making progress on removing Immutable and also converting Flux stores to Redux 🥇

After looking at this PR, I concluded I would be perfectly comfortable reviewing one big PR that does the whole migration at once.

The most tricky part (toggle-state) is already done here, and the rest are rather simple flags to tracking fetching status etc.

Then the store can be converted to Redux and the usage in UI can stop subscribing to store updates and start connecting to Redux and calling selectors (these need to be written) instead of state.settings.get( option ) etc.

What do you think?

jsnajdr · 2018-11-22T12:52:38Z

client/lib/notification-settings-store/test/toggle-state.js

+						},
+					},
+				},
+			};


An idea to make the tests less verbose and repetitive: create helper for creating a mock state:

const startingState = createState( [ 'settings', 'dirty', 'wpcom', settingToToggle ], true );

and check the expected value by:

expect( get( state, [ 'settings', 'dirty', 'wpcom', settingToToggle ] ) ).toBe( false );

extracting a path variable makes this particular test even shorter:

const path = [ 'settings', 'dirty', 'wpcom', settingToToggle ];

I tend to prefer avoiding doing any abstractions in the tests if it can be helped, instead opting for each test to show as clearly as possible and with as little indirection as possible what the input and output should be. I like the idea of createState somewhat but I'd still prefer to not have the future reader have to look at what createState is doing and how it relates to the test.

In terms of check with expect( get( state, ... ) ).toBe( false ); I'd also err away from such a direct check. Reason being that our unit logic might change by mistake to add other things to the object. The settingToToggle might be successfully toggled but other side effects could sneak in unchecked.

That said, I've changed the responsibility of toggle-state.js a little and so now it returns the minimum object shape needed (i.e. no extra settings.dirty.x), so that's an improvement at least :)

Hope that makes sense and is reasonable?

I think that tests should be written the same way as any other program. Avoid repetitive code, extract it to functions. Test the program behavior (did it toggle the setting?) rather than internal implementation (exact shape of the state object).

A good criterion for a well written test would be: if I make one-line change in the implementation, is the test update also an one-line change? Or do I need to update 50 repetitive tests?

I'm just offering a personal opinion here, no need to update the PR if you don't agree enthusiastically 🙂

jsnajdr · 2018-11-22T12:58:35Z

client/lib/notification-settings-store/toggle-state.js

+				],
+			},
+		},
+	};


Use combineReducers and keyedReducer to avoid this horrible object spread chain:

combineReducers ( { settings: combineReducers( { dirty: combineReducers( { blogs } ) } ) } )

Now, it's the blogs reducer that contains the code that really matters. And it's the first reducer in the chain that needs to run the isNaN( stream ) condition.

Don't write repetitive code -- reorganize it instead and create helpers.

jsnajdr · 2018-11-24T08:19:38Z

client/lib/notification-settings-store/toggle-state.js

+} );
+
+const toggleInDevice = ( devices, deviceId, setting ) => {
+	const device = find( devices, ( { device_id } ) => device_id === parseInt( deviceId, 10 ) );


Nit:

const deviceIdNum = parseInt( deviceId, 10 ); const device = find( devices, { device_id: deviceIdNum } );

parses the string into a number only once and uses lodash.find shorthand for matching a property value.

Good spot :)

jsnajdr · 2018-11-27T12:40:59Z

client/me/notification-settings/blogs-settings/header.jsx

+		const filteredSettings = {
+			...omit( settings, [ 'blog_id', 'devices' ] ),
+			email: omit( get( settings, 'email', {} ), 'achievement' ),
+		};


Lodash omit can also remove nested properties:

omit( settings, [ 'blog_id', 'devices', 'email.achievement' ] ); omit( settings, [ 'blog_id', 'devices', [ 'email', 'achievement' ] ] );

TIL :) Lodash always pleasantly surprises 👌

This could still be one omit call with array of nested properties. Or did you choose to keep the code as it is?

jsnajdr · 2018-11-27T13:06:30Z

client/me/notification-settings/blogs-settings/header.jsx

+			// 		 - timeline.store_order
+			// 		 - email.store_order
+			// 		 - email.scheduled_publicize
+			//		 Should we filter these also?


Notifications exceptions are defined here. Should we use this constant also to filter notification types in the getLegend function?

timeline.store_order is a valid notification, but only present on sites that have a Woo store. See #19196 -- a small PR where the store_order notification is added.

I don't know why some notification emails are disabled, namely Achievements, Publicize events and Store Orders. Pinging people who worked on the more recent additions: @artpi (publicize) and @justinshreve (store).

Store order emails are disabled because WooCommerce currently handles and sends out its own suite of email notifications. These are not tied in with WordPress.com's email/notification system, and we didn't want to implement double notifications.

Thanks for the info @justinshreve - for now I'll update the logic to omit those items. The only consequence in this section is the copy shown in the section header. I'll leave a ref to this comment thread too.

jsnajdr · 2018-11-27T13:20:48Z

client/me/notification-settings/blogs-settings/header.jsx

+
+		const [ onSettings, offSettings ] = partition(
+			flatten( [ ...map( filteredSettings, values ), ...map( devicesSettings, values ) ] )
+		);


_.countBy could be useful: the result is [ onSettingsCount, offSettingsCount ] instead of two arrays. Better for our purpose here.

By the way, this toggle-counting code would be more readable if it had some comments that explain the shape of the arrays and objects being processed and what is being done to them.

I think my Lodash-fu is quite good, but I still had serious trouble figuring out what all these Lodash omit -> map -> values -> flatten -> partition transforms do. The most difficult question is: "What shape does this particular object have? Is it array? Array of what? Object? What are the keys and values?"

jsnajdr · 2018-11-27T13:29:24Z

client/me/notification-settings/settings-form/stream.jsx

@@ -37,7 +33,7 @@ class NotificationSettingsFormStream extends PureComponent {

 		if ( this.props.devices && size( this.props.devices ) > 0 ) {
 			stream = parseInt( this.state.selectedDeviceId || first( this.props.devices ).id, 10 );
-			settings = this.props.settings.find( device => device.get( 'device_id' ) === stream );
+			settings = this.props.settings.find( ( { device_id } ) => device_id === stream );


settings = find( settings, { device_id: stream } );

I was very close to pulling in find here! Makes sense to have it follow what we see in similar files

jsnajdr · 2018-11-27T13:30:20Z

client/me/notification-settings/main.jsx

-			this.state.settings.find( blog => blog.get( 'blog_id' ) === parseInt( blogId, 10 ) );
-		const onSave = blogId => saveSettings( 'blogs', findSettingsForBlog( blogId ) );
-		const onSaveToAll = blogId => saveSettings( 'blogs', findSettingsForBlog( blogId ), true );
+			this.props.settings.find( ( { blog_id } ) => blog_id === parseInt( blogId, 10 ) );


Use the find( this.props.settings, { blog_id } ) shorthand.

jsnajdr · 2018-11-27T13:37:53Z

client/state/notification-settings/actions.js

+		.updateNotificationSettings(
+			buildSavePayload( source, settings ),
+			applyToAll,
+			( error, data ) => {


Use promise version here and avoid the if/else conditional.

jsnajdr · 2018-11-27T13:46:09Z

client/state/notification-settings/toggle-state.js

+
+		return {
+			blogs: [
+				...without( blogs, blog ),


Toggling a setting in an array by removing the old one and appending the updated one to the end has one disadvantage: when I toggle a setting on and the off again, I expect _.isEqual( oldArray, newArray ) to be true. It works for objects, as the order of properties doesn't matter, but doesn't work for arrays. [ 1, 2 ] is not equal to [ 2, 1 ].

Ahh good point! Instead we could slice to clone immutably and mutate the clones value.

jsnajdr · 2018-11-27T13:47:57Z

client/state/notification-settings/toggle-state.js

+		};
+	},
+
+	blog( state, source, stream, setting ) {


Can you rename this to blogs? The state stores the blogs settings as blogs, the getNotificationSettings selector is always called with blogs parameter... I don't know how this inconsistency was introduced, but I believe it's time to fix it.

jsnajdr · 2018-11-27T13:49:25Z

client/state/notification-settings/reducer.js

+		return toggleState[ source ].apply( this, arguments );
+	}
+
+	return toggleState.blog.apply( this, arguments );


The toggleState.blog function should be renamed to toggleState.blogs and then we can always use the toggleState[ source ] version.

I definitely want to get rid of the need for this intermediate toggleSetting step and instead have the code be more explicitly defined.

It turns out that this is somewhat necessary, or at least convenient... Sometimes the source is a number (blog id). I'm going to leave this as is for now just because I'm not sure how far reaching any changes would be :/

Oh, now I see the code that passes blogId as source, too. Makes sense 👍

jsnajdr · 2018-11-27T13:55:41Z

client/lib/notification-settings-store/test/toggle-state.js

+						},
+					},
+				},
+			};


I think that tests should be written the same way as any other program. Avoid repetitive code, extract it to functions. Test the program behavior (did it toggle the setting?) rather than internal implementation (exact shape of the state object).

A good criterion for a well written test would be: if I make one-line change in the implementation, is the test update also an one-line change? Or do I need to update 50 repetitive tests?

I'm just offering a personal opinion here, no need to update the PR if you don't agree enthusiastically 🙂

spen

Thanks for another round of reviewing @jsnajdr - much appreciated!
I'll get to fixing up those items by the end of the week :)

spen · 2018-11-27T14:37:57Z

client/state/notification-settings/actions.js

+	wpcom
+		.undocumented()
+		.me()
+		.getNotificationSettings( ( error, data ) => {


Agreed! I figured I'd just get it copied through but it is technically a new file - I'll update soon :)

spen · 2018-11-27T14:39:07Z

client/state/notification-settings/actions.js

+			return {
+				other: settings,
+			};
+		default:


I thought so too and I've made some note of this. I haven't purposely tested that change yet though but I'll update it once I have :)

spen · 2018-11-27T14:41:44Z

client/state/notification-settings/reducer.js

+		return toggleState[ source ].apply( this, arguments );
+	}
+
+	return toggleState.blog.apply( this, arguments );


I definitely want to get rid of the need for this intermediate toggleSetting step and instead have the code be more explicitly defined.

spen · 2018-11-27T14:44:09Z

client/state/notification-settings/toggle-state.js

+
+		return {
+			blogs: [
+				...without( blogs, blog ),


Ahh good point! Instead we could slice to clone immutably and mutate the clones value.

…e on Immutable

… bookends logic

…lter extra settings

jsnajdr

Let's fix one prop type warning and have a closer look at replaceAtIndex when an index can't be found. Other than that, I think we're almost ready to 🚢

jsnajdr · 2018-12-13T12:02:25Z

client/me/notification-settings/blogs-settings/header.jsx

+		const filteredSettings = {
+			...omit( settings, [ 'blog_id', 'devices' ] ),
+			email: omit( get( settings, 'email', {} ), 'achievement' ),
+		};


This could still be one omit call with array of nested properties. Or did you choose to keep the code as it is?

jsnajdr · 2018-12-13T12:40:24Z

client/me/notification-settings/settings-form/stream.jsx

-			PropTypes.instanceOf( Immutable.List ),
-			PropTypes.instanceOf( Immutable.Map ),
-		] ).isRequired,
+		settings: PropTypes.object.isRequired,


This prop type should be "array or object". When rendering settings for mobile devices, the settings prop is an array of object (one for each device). And it's just one object otherwise.

See also how the getStreamSettings method (few lines below) behaves differently when props.devices is present.

I found this bug when testing and seeing a prop type warning from React in console.

jsnajdr · 2018-12-13T12:43:29Z

client/state/notification-settings/toggle-state.js

+	const deviceSetting = get( device, setting );
+
+	return {
+		devices: replaceAtIndex( devices, deviceIndex, {


What if deviceIndex is -1, i.e., the device is not yet in the array? The old code supported the "replace or add" behavior, didn't it?

It seems that it doesn't matter, as the REST API response (and Redux state) has fields for all possible settings for all your blogs and devices. Therefore a field always exists when we toggle it. But I'm not sure and the previous version of the code was more forgiving.

…ncluded

jsnajdr

Thanks for working on this big migration and addressing all review comments. Let's 🚢

spen · 2018-12-14T12:55:20Z

Heaps of thanks for reviewing the different iterations of this and offering up some nice tips and insight along the way! 🙏💚

spen added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Type] Janitorial labels Nov 18, 2018

spen mentioned this pull request Nov 18, 2018

Framework: Remove Immutable #28652

Closed

7 tasks

spen force-pushed the update/refactor-notifications-toggle-state branch 2 times, most recently from 9eca5b0 to efdf1e4 Compare November 19, 2018 04:29

jsnajdr reviewed Nov 22, 2018

View reviewed changes

spen mentioned this pull request Nov 23, 2018

Reduxify: notifications-settings-store #24179

Closed

spen force-pushed the update/refactor-notifications-toggle-state branch from efdf1e4 to 6f2db3e Compare November 23, 2018 16:18

jsnajdr reviewed Nov 24, 2018

View reviewed changes

spen added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 26, 2018

spen changed the title ~~Refactor notifications toggleState~~ Framework: Convert NotificationsSettings store to redux & remove Immutable from module. Nov 26, 2018

spen force-pushed the update/refactor-notifications-toggle-state branch from 6e59d32 to 8eaaf3c Compare November 26, 2018 18:05

spen added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 26, 2018

jsnajdr reviewed Nov 27, 2018

View reviewed changes

spen commented Nov 27, 2018

View reviewed changes

spen added 10 commits December 11, 2018 17:00

Notifications: Add tests for toggleState.wpcom

ab4ac93

Notifications: Add tests for toggleState.other

a19a0ec

Notifications: Add tests for toggleState.blog

d14f862

Notifications: Refactor toggle-state to be have only a slim dependanc…

fe2cdc1

…e on Immutable

Notifications: Move Immutable conversion out of toggle-state

7676fe3

Notifications: Refactor notifactions-settings-store so Immutable only…

87be1d9

… bookends logic

Refactor getLegend to not rely on Immutable logic

a4be4c3

NotificationSettings: Remvoe Immutable from module

9b1289c

NotificationSettings: Update toggleState to be slimmer

f151f98

NotificationSettings: Replace flux store with redux state

9c0dc64

spen added 5 commits December 11, 2018 17:00

NotificationsSettings: Write tests for maintaining array orders

24fe124

NotificationsSettings: add and use replaceAtIndex util

f746081

NotificationsSettings: use _.find to make use of property shorthands

316eb84

NotificationsSettings: use promised api call

ddcece3

NotificationSettings: Use countBy for calculating the header text, fi…

56f5063

…lter extra settings

spen force-pushed the update/refactor-notifications-toggle-state branch from 8eaaf3c to 56f5063 Compare December 11, 2018 17:04

jsnajdr reviewed Dec 13, 2018

View reviewed changes

spen added 3 commits December 13, 2018 15:16

NotificationSettings: Improve array handling to append if item isnt i…

2c4d5dc

…ncluded

NotificationSettings: Update proptype in settings-form/stream

95c6bbf

NotificationSettings: Improve omit logic in header.jsx

02558f8

spen force-pushed the update/refactor-notifications-toggle-state branch from 1bfdf87 to 02558f8 Compare December 13, 2018 17:37

jsnajdr approved these changes Dec 14, 2018

View reviewed changes

spen merged commit b8a3d4d into master Dec 14, 2018

spen deleted the update/refactor-notifications-toggle-state branch December 14, 2018 12:54

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 14, 2018

Framework: Convert NotificationsSettings store to redux & remove Immutable from module. #28653

Framework: Convert NotificationsSettings store to redux & remove Immutable from module. #28653

Conversation

spen commented Nov 18, 2018 • edited Loading

Changes proposed in this Pull Request

Testing instructions

Notes:

blogs/blog in toggleState

matticbot commented Nov 18, 2018

jsnajdr commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr Nov 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr left a comment

Choose a reason for hiding this comment

spen commented Dec 14, 2018

spen commented Nov 18, 2018 •

edited

Loading

`blogs`/`blog` in `toggleState`

jsnajdr Nov 24, 2018 •

edited

Loading

jsnajdr Nov 27, 2018 •

edited

Loading