Media: Clear icon site value and setting upon deleting associated media #10016

aduth · 2016-12-13T21:14:25Z

This pull request seeks to clear site icon values (both on the site object itself and the site setting) when deleting a media item which is currently assigned as the site icon. Server-side, the option is automatically cleared when the media is deleted, so we only need to recreate this behavior in the client (related source).

Testing Instructions:

Testing will be made easier by use of Redux DevTools Chrome extension.

Navigate to Site Settings
Select a site
Assign a site icon by clicking "Change Site Icon" (select image, "Continue", crop, "Done")

This will set the site setting value, but does not yet change front-end icon shown

Click "Change Site Icon" again
Choose the newly created cropped image
Click Delete
In developer tools Redux tab, note that...

Upon the MEDIA_DELETE dispatched action, the diff shows that the state.siteSettings.items[ siteId ].site_icon was set to null
A RESET_SITE_ICON action is also dispatched, and the diff shows that the state.sites.items[ siteId ].icon was removed (but only if you have a site icon already assigned via non-Calypso flow)

You can also verify that deleting any other media will dispatch MEDIA_DELETE with no effect, and will not dispatch RESET_SITE_ICON.

matticbot · 2016-12-13T21:14:26Z

Test live: https://calypso.live/?branch=update/media-delete-reset-site-icon

youknowriad · 2016-12-14T11:34:57Z

client/post-editor/media-modal/index.jsx

+		this.props.deleteMedia( site.ID, mediaIds );
+		if ( includes( mediaIds, siteIconId ) ) {
+			this.props.resetSiteIcon( site.ID );
+		}


I'm a bit concerned about this code, what if we allow media deletion from elsewhere, we'll face the exact same problem. Do we have the media URLs available here, that way we could use them to compare against the siteIcon attribute in the site reducer?

Or we could solve this differently, move this "extra" behavior to the deleteMedia action creator. You could call to requestSiteSettings action creator from there. I know it's not perfect, but maybe this could allow us to "centralize" this behavior, which seem not directly related to this component? Thoughts? I may be missing something tho :)

Yeah, I had a hard time coming to a solution here. It's complicated by the fact that the data exists in many places (media objects themselves, a media ID as site setting, and a media's URL as the site icon property). But you're right that any part of the app which deletes a media would then need to be concerned with resetting the site icon, which is not great.

Similar to what I suggested at #10032 (comment), perhaps a middleware (the API middleware?) is the right place for these kinds of side effects. While we're not quite at the point where we want to substitute the API call for deleting media, from there we could at least check whether the media to be deleted matches the current site option.

Or, maybe since it's not related to network calls on its own, it could exist as a standalone middleware, or combined with other media middleware suggested in #9706.

cc @dmsnell in case you have thoughts on the role of middlewares / API middleware

@aduth middleware is probably a reasonable solution for this, but it would seem a bit odd with the deleteMedia action doing the deletion and leaving the site icon in middleware. maybe not. maybe it would actually be a quick and easy function to add to the wpcom data layer.

in essence, we should ask if deleting the media item currently set as the site icon always means that the icon should be reset. if this is the case, I would expect to have some siteIcon reducer somewhere in the state tree listening for mediaDelete actions and updating in Calypso-first by resetting it. in other words, deleteMedia would not be the genesis for resetting the site icon. this is by design in Redux. the siteIcon state/reducer is responsible for its own behavior and if the proposition holds, it should be listening for all relevant actions which affect it.

the middleware comes in as the data layer. we act as if Calypso is the raw source of truth. the site icon reducer resets its value when it see MEDIA_DELETE with its own media id. the middleware layer for the site icon (or site setting) endpoint also listens for this same action and will trigger an API call whenever one flies by. if the API call fails, it may very well reset the site icon to its previous id, or in this case, if the media is gone, the failure mode will do something else - set some default for example.

does this help?

youknowriad · 2016-12-14T11:38:36Z

client/state/media/actions.js

+export function deleteMedia( siteId, mediaIds ) {
+	return {
+		type: MEDIA_DELETE,
+		mediaIds: castArray( mediaIds ),


a new Lodash tip learned for me :)

aduth · 2016-12-15T15:58:54Z

A thought which might help with reconciling the icon formats is including the ID of the media item associated with the site icon on the site response, perhaps as site.icon.media_id. This seems like a reasonable addition anyways, as it would be useful as a pointer in case the client wants more information about the particular media item used as site icon (could include said pointer request in site.meta.links).

Then, as much as possible, we use the site icon ID as the source of truth, with a lookup of the associated media item if available in the current state, falling back to the site object's icon.img property. Ideally we would just work with the ID, rendering a <QueryMedia id={ id } /> component to fetch the associated media in order to get its URL, but since icons are shown in the site selector, this might be unreasonably network intensive.

I'm still working on an implementation to see whether this will pan out, but figured I'd throw the idea out there.

dmsnell · 2016-12-15T18:22:55Z

but since icons are shown in the site selector, this might be unreasonably network intensive.

as an aside, this would be a good use for the API middleware. no need to refetch icons within a certain time, maybe within any given session. the middleware could what which ones have been fetched and ignore new requests for the same one.

I've been thinking about a force protocol overall for cases where we want to refresh for sure, which I think would be well suited for some action meta

aduth · 2016-12-19T20:48:13Z

Ok, took me a little longer than I'd hoped to implement the approach suggested in my previous comment, but is now reflected in the last handful of commits. It's a reasonable departure from the previous direction. Instead of assigning an icon URL to be tracked in site state, it's now assumed that the site items are aware of the media ID associated with the site icon by its icon.media_id property*. Until media is properly Reduxified, we receive the saved item into state manually after having cropped the selected image for icon. Then, when updating the site settings, the reducer for site updates its own state to use this ID. When <SiteIcon /> renders next, it tries to look up the icon URL using the site and media state. When deleting, the sites reducer only needs to check whether the IDs of the items being deleted includes the currently assigned icon ID.

* This is not yet passed by the API, and I plan to work on this next. Though for our purposes this works fairly well as-is, given fallbacks and icon updates within browser state. Only problem is if the site is received (e.g. polling) after the icon had been updated in the same session.

aduth · 2016-12-19T20:52:32Z

Also, yes, the changes are quite large, and there's a decent amount of overlap with related pull requests (particularly media state). Hoping with some merging / rebasing it will come down to a more reasonable size.

aduth · 2016-12-19T21:52:02Z

D2940-code

youknowriad · 2016-12-20T08:54:18Z

client/components/site-icon/index.jsx

+export default connect( ( state, { site, siteId, imgSize } ) => {
+	// Until sites state is completely within Redux, we provide compatibility
+	// in cases where site object is passed to use the icon.img property as URL
+	if ( site ) {


Just asking, one could refactor the parent component and provide a "redux" site instead no?

Just asking, one could refactor the parent component and provide a "redux" site instead no?

In some cases, yes. But only for sites we know to be in state, which at the moment only includes the currently selected site. Since site icons are shown in the site selector, we can't yet use Redux state there.

youknowriad · 2016-12-20T09:15:36Z

client/state/sites/reducer.js

-		case SITE_ICON_SET: {
-			const { siteId, iconUrl } = action;
+		case SITE_SETTINGS_UPDATE:
+		case SITE_SETTINGS_RECEIVE: {


So, If I understand properly we're receiving the site Icon ID from the site settings endpoint, I'm wondering why we're storing it in this reducer, instead of relying on the siteSettings state to handle it (like other settings)?

Nevermind, after review the API diff, I understand why you had to do that :)

So, If I understand properly we're receiving the site Icon ID from the site settings endpoint, I'm wondering why we're storing it in this reducer, instead of relying on the siteSettings state to handle it (like other settings)?
Nevermind, after review the API diff, I understand why you had to do that :)

Yeah, with these changes, we're treating site.icon.media_id as the preferred source of truth for the site icon, which means we want to reconcile when incoming settings are received. Now that you mention it though, this might need to go both ways: when we receive site(s), and we know those sites in the siteSettings reducer, we'll probably want to update the site icon value there (though this likely would have little effect since, as I mentioned, we want to prefer using it on the site state anyways).

we're treating site.icon.media_id as the preferred source of truth for the site icon, which means we want to reconcile when incoming settings are received

beautiful. I love seeing these changes happen

You mentioned in a chat that you prefer to keep the 'icon' attribute on the site endpoint to avoid extra requests. But on the same time, you're considering here site.icon.media_id as the preferred source of truth (which I agree with).

Maybe we should not have to care about the extra requests here (unless it's problematic, which I don't think it is), and maybe leave the DataLayer optimize this later.

Also, if the media_id is some general information about the site, we should add it to the site endpoint directly, don't you think?

@youknowriad there was a distinction made a long time ago between site and siteSettings on the API-side wherein site requests return quick data but settings request get data that is much slower to fetch on the server side. it's unlikely right now that this will change.

leave the DataLayer optimize this later

I agree with this and think it's a great place to introduce the HTTP request batching, but there are steps involved there, so maybe some clever logic in the API layer can get us there in the meantime

Also that /sites/%s/settings is read/write while /sites/%s is read-only.

youknowriad · 2016-12-20T09:29:52Z

client/components/site-icon/index.jsx

+	// exclusively using the site ID
+	return {
+		site: getSite( state, siteId ),
+		iconUrl: getSiteIconUrl( state, siteId, imgSize )


This selector relies on two sub-state trees: sites and media, Shouldn't we add Queries to the component? probably a QuerySites since "in theory" the component could get any siteId as prop. I and guess QueryMedia is probably not available yet and receiveMedia is only called if we use the SiteIconSetting component, so I wonder how is it filled in that case?

This selector relies on two sub-state trees: sites and media, Shouldn't we add Queries to the component?

You raise a good point here. In an earlier comment, I noted a worry about the number of requests from <SiteIcon /> components since they're used for each of the user's sites in the site selector.

If we were to render a <QuerySites />, maybe it should be limited to the condition that the site data is not yet known (and that it's passed as a site ID, i.e. Redux)?

Regarding <QueryMedia />, we benefit from the fallback behavior which uses the site's site.icon.img property (a URL string) if the media by ID is not known. The ID only changes when updating the site icon within Calypso, at which point we've received the media explicitly (in <SiteIconSetting />). I think this should be enough that it would never be inaccurate, though admittedly depends on a tangled set of assumptions.

ytihhrt

??

ytihhrt

??

😆

Corrected to "requests".

If we were to render a , maybe it should be limited to the condition that the site data is not yet known (and that it's passed as a site ID, i.e. Redux)?

Why is this? ~~Is it for data usage? If so, maybe we should be able to do a new fetch just for the site info data.~~ This would be trivial in the new API data layer since we could trap some action such as SITE_ICON_REQUEST and make an API call presumably to the same site settings endpoint from where it comes but then limiting the response fields to only the site icon data.

Sounds like it's time sooner than later to introduce the HTTP layer which turns a sequence of rapid-fire HTTP requests into a BATCH request

Why is this?

Mainly because otherwise every time I open my site selector, it would issue about 30 requests for each site shown, even if we already have data for those sites from /me/sites. We have this implied assumption in Calypso that sites data is expected to be universally requested via /me/sites, not specifically through any one query component. Not that this is very good, mind you, but the current situation nonetheless.

youknowriad · 2016-12-23T08:36:23Z

While testing the API side, I noticed that for WPCOM Sites using Site Icon (and not blavatar) don't have the icon property on the /sites/%s endpoint. Should wa add a <QuerySiteSettings> to make sure this usecase is covered by this PR?

youknowriad

@aduth I took this for a spin again and It's working as expected. And as you said, the API diff in code-D2990 will add the icon property when relying on the site-icon setting.

aduth · 2016-12-23T15:22:12Z

Ah, sorry I was a bit late in pushing my rebase @youknowriad . Not too much has changed at a high level, but specifically:

Reorganized, squashed, and shuffled changes around in commit history
Opted to remove keyedReducer for now since I do not want to persist media (yet) and this is not currently possible with the utility. Instead reimplemented reducers manually keying by siteId.
Moved selectors to global selectors directory
Split off getSiteIconId into distinct selector. This is a tad granular for my liking, but I think is important to ensure we have consistent retrieval of site ID.

To expand on the last point, I had mentioned that we may need to add handlers to site settings to update the site_icon properties for sites it tracks, but in retrospect, if we are to treat sites as the preferred source for icon ID, this is unnecessary. Using a selector should discourage developers from trying to access the site setting's site_icon directly.

aduth · 2016-12-23T15:25:28Z

client/state/site-settings/reducer.js

@@ -64,7 +66,21 @@ export const items = createReducer( {}, {
 			...state[ siteId ],
 			...settings
 		}
-	} )
+	} ),
+	[ MEDIA_DELETE ]: ( state, { siteId, mediaIds } ) => {


Hmm, given my note at #10016 (comment), maybe we don't even need to handle this.

If I understand correctly, you're saying that we always rely on the site object to retrieve the siteIcon. Thus, we should not worry about resetting it on the siteSettings? Is this assumption correct? Maybe we should consider dropping the selector fallback then, since the site-settings value could be incorrect (even if in the current workflow, it can never happen since the site object will be available in this case) ?

You raise a good point. The fallback exists in case we ever receive site settings before the site itself. But if that were ever to occur, we would still want to monitor for media deletions even if we only knew the site settings. But we still do not need to account for SITE_RECEIVE or SITES_RECEIVE in the site settings reducer because at that point we can assume that the fallback will no longer be used.

Because of this, I'll plan to drop 15cb3f5 in a rebase and revert back to this previous behavior.

youknowriad · 2016-12-23T15:33:50Z

client/state/selectors/get-media-item.js

@@ -0,0 +1,16 @@
+/**


Great to see selectors making their way to the global directory

youknowriad · 2016-12-23T16:19:32Z

I tested after the rebase and it's still working properly

aduth added [Feature] Media The media screen in Calypso, general media management, or integration with third party media. Sites [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Dec 13, 2016

aduth added this to the Site Icon milestone Dec 13, 2016

aduth requested a review from youknowriad December 13, 2016 21:15

youknowriad reviewed Dec 14, 2016

View reviewed changes

aduth force-pushed the update/media-delete-reset-site-icon branch from 4435341 to b68534c Compare December 19, 2016 20:33

youknowriad reviewed Dec 20, 2016

View reviewed changes

youknowriad approved these changes Dec 23, 2016

View reviewed changes

aduth force-pushed the update/media-delete-reset-site-icon branch from ebc2890 to 97159f9 Compare December 23, 2016 15:15

aduth commented Dec 23, 2016

View reviewed changes

youknowriad reviewed Dec 23, 2016

View reviewed changes

aduth force-pushed the update/media-delete-reset-site-icon branch from 15cb3f5 to 5ed530e Compare December 23, 2016 17:48

This was referenced Dec 28, 2016

Reduxify MediaStore #8625

Closed

Reduxify Media Library #10352

Closed

aduth added 3 commits January 2, 2017 09:38

Site Icon: Track site icon by received media ID on site, site settings

bf67e16

Media: Reset site icon upon deleting media

6cbbf8a

Components: Display site icon by performing media lookup of icon ID

b9740a2

aduth force-pushed the update/media-delete-reset-site-icon branch from 5ed530e to b9740a2 Compare January 2, 2017 15:13

aduth added the [Status] Ready to Merge label Jan 2, 2017

aduth removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 2, 2017

State: Define numeric media ID in site schema icon

81134d3

aduth merged commit 19cd141 into master Jan 2, 2017

aduth deleted the update/media-delete-reset-site-icon branch January 2, 2017 15:50

aduth mentioned this pull request Jan 2, 2017

Framework: Overload resizeImageUrl to use service detection #10264

Merged

ellatrix mentioned this pull request Jan 3, 2017

Add QueryMedia component connected to a Redux store. #10392

Merged

3 tasks

aduth added the [Feature] Site Settings All other general site settings. label Jan 6, 2017

aduth mentioned this pull request Jan 6, 2017

Site Settings: Show indication that site icon upload is in progress #10452

Closed

lancewillett removed the [Status] Ready to Merge label Jan 18, 2017

aduth mentioned this pull request Mar 1, 2017

Editor: Hide SEO description for private/hidden sites #11579

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Media: Clear icon site value and setting upon deleting associated media #10016

Media: Clear icon site value and setting upon deleting associated media #10016

aduth commented Dec 13, 2016 •

edited by artpi

Loading

matticbot commented Dec 13, 2016

youknowriad Dec 14, 2016

aduth Dec 14, 2016

dmsnell Dec 14, 2016

youknowriad Dec 14, 2016

aduth commented Dec 15, 2016

dmsnell commented Dec 15, 2016

aduth commented Dec 19, 2016

aduth commented Dec 19, 2016

aduth commented Dec 19, 2016

youknowriad Dec 20, 2016

aduth Dec 20, 2016

youknowriad Dec 20, 2016

youknowriad Dec 20, 2016

aduth Dec 20, 2016

dmsnell Dec 20, 2016

youknowriad Dec 21, 2016

dmsnell Dec 21, 2016

aduth Dec 21, 2016

youknowriad Dec 20, 2016

aduth Dec 20, 2016 •

edited

Loading

dmsnell Dec 20, 2016

aduth Dec 20, 2016

dmsnell Dec 20, 2016 •

edited

Loading

aduth Dec 22, 2016

youknowriad commented Dec 23, 2016

youknowriad left a comment

aduth commented Dec 23, 2016

aduth Dec 23, 2016

youknowriad Dec 23, 2016

aduth Dec 23, 2016 •

edited

Loading

youknowriad Dec 23, 2016

youknowriad commented Dec 23, 2016

Media: Clear icon site value and setting upon deleting associated media #10016

Media: Clear icon site value and setting upon deleting associated media #10016

Conversation

aduth commented Dec 13, 2016 • edited by artpi Loading

matticbot commented Dec 13, 2016

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

aduth commented Dec 15, 2016

dmsnell commented Dec 15, 2016

aduth commented Dec 19, 2016

aduth commented Dec 19, 2016

aduth commented Dec 19, 2016

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

aduth Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmsnell Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Dec 23, 2016

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Dec 23, 2016

aduth commented Dec 13, 2016 •

edited by artpi

Loading

aduth Dec 20, 2016 •

edited

Loading

dmsnell Dec 20, 2016 •

edited

Loading

aduth Dec 23, 2016 •

edited

Loading