Skip to content
This repository has been archived by the owner on Aug 24, 2018. It is now read-only.

Handle deletion of attachment associated with media widget #27

Merged
merged 16 commits into from
Mar 21, 2017

Conversation

obenland
Copy link
Contributor

@obenland obenland commented Mar 16, 2017

Shows an error message in the media widget when its associated attachment is deleted.

Also adds a label to the attachment in the media list, when its used in a widget.

Fixes #22.

/cc @timmyc

@obenland obenland added the bug label Mar 16, 2017
@obenland obenland requested a review from westonruter March 16, 2017 18:29
*/
public function display_media_state( $states, $post ) {
$settings = $this->get_settings();
$attachment_id = empty( $settings[ $this->number ]['attachment_id'] ) ? 0 : $settings[ $this->number ]['attachment_id'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only going to work if you have one image widget. If you have multiple image widgets, then you'd have to iterate over all $settings to check each one to see if it has attachment_id that is the same as $post->ID. As it is right now, it's only looking at the current $this->number widget instance, which is probably going to be the last one registered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in db98115.

* @param WP_Post $post The current attachment object.
* @return array
*/
public function display_media_state( $states, $post ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this maybe be moved to the base WP_Widget_Media class? Naturally some of the hard-coded values here would need to change, like replace 'image/' with $this->widget_options['mime_type'] and use $this->name instead of 'Image Widget'.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use $this->name instead of 'Image Widget'

Duh! I didn't think of that. Yes it should be in WP_Widget_Media, the only reason it's not is because I didn't want a hard coded array of names. So yeah, that solves it.

Copy link
Contributor

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something else related to the behavior of a widget upon deletion of its associated attachment is what happens when you try deleting the image used for the image widget you're currently editing. See https://cloudup.com/cJs4gRGYnXE

Per https://github.com/xwp/wp-core-media-widgets/pull/27/files#r106510104 I think this should result in the selected image being removed from the widget, and a notification shown that the image was removed.

*/
public function delete_attachment_data( $post_id ) {
$settings = $this->get_settings();
$attachment_id = empty( $settings[ $this->number ]['attachment_id'] ) ? 0 : $settings[ $this->number ]['attachment_id'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As done in display_media_state , this will need to loop over all $settings not just $settings[ $this->number ].

*
* @param int $post_id Attachment ID.
*/
public function delete_attachment_data( $post_id ) {
Copy link
Contributor

@westonruter westonruter Mar 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deleting the widget instances when the associated attachment is deleted makes me nervous. I think it would be better to instead cause the widget to not render anything, and show a notification in the widget control that the attachment has been removed. For example, I think it should behave like an RSS widget pointing to a 404 feed URL.

By showing a notice that the attachment has been deleted, there would be less user mystery when they find a widget disappeared. Deleting the widget would also cause the widget's title to be dropped, resulting in data loss. I think it would be better if a user discovers they deleted an image used by an image widget, and then picks another image for that existing widget, rather than having to create a whole new widget from scratch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➕ I was thinking the same thing - when the media item is deleted, perhaps just not render anything - or perhaps if the user has a certain role, and is viewing the site, then show a notice in the widget area that the attachment is missing.

obenland added a commit that referenced this pull request Mar 17, 2017
obenland added a commit that referenced this pull request Mar 17, 2017
@obenland
Copy link
Contributor Author

obenland commented Mar 17, 2017

7d566e9 introduced an error message when an attachment can't be loaded:

screen shot 2017-03-17 at 8 57 17 am

@melchoyce What should the message look like and say?
@westonruter @timmyc Is this how you would go about it? Setting an error on the model? Also: What other instances can you think of where the attachment can't be displayed?

EDIT: We should probably not display the Edit button in case of an error either.

@obenland obenland force-pushed the fix/delete-attachment branch from 7d566e9 to 15f66db Compare March 17, 2017 16:09
obenland added a commit that referenced this pull request Mar 17, 2017
obenland added a commit that referenced this pull request Mar 17, 2017
@melchoyce
Copy link
Contributor

melchoyce commented Mar 17, 2017

How about:

We can't find that image. Check your media library and make sure it wasn't deleted.

We'll also want to make sure the text is left-aligned.

(Props @michelleweber)

obenland added a commit that referenced this pull request Mar 17, 2017
@obenland
Copy link
Contributor Author

Thank you! Updated in c5bc6d5

@obenland
Copy link
Contributor Author

@westonruter: Using the widget name doesn't work too well:

screen shot 2017-03-17 at 10 49 01 am

I'm not sure how I feel about sprintf( __( '%s Widget' ), $this->name ), even with a translator comment. Nesting translated strings like that usually doesn't go over well.

@westonruter
Copy link
Contributor

@obenland oh I forgot that “widget” wasn't actually part of the name. Two options I can think of:

  1. We change the translation string to be more robust for translation purposes, like __( 'Media widgets for "%s"' ).
  2. We introduce new l10n label for this purpose, as we already have for defining the strings used for the buttons. So then we'd use something like $this->l10n['media_library_display_state'] in this case.

I think 2 would be a better choice.

@obenland
Copy link
Contributor Author

I think 2 would be a better choice.

Agreed

@@ -33,6 +33,10 @@ public function __construct() {
'edit_media' => __( 'Edit Image' ),
'change_media' => __( 'Change Image' ),
'select_media' => __( 'Select Image' ),
'error' => sprintf(
__( 'We can&#8217;t find that image. Check your <a href="%s">media library</a> and make sure it wasn&#8217;t deleted.' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Backbone view for should have a new event handler for clicking on this link to have the same result as clicking the "Select Media" or "Change Media" buttons. The underlying href can remain there so they can right-click to open the media library in a new tab, but otherwise we should keep them inside the customizer and open the select frame for the media modal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in 43e5c93.
I decided to go with a new selector that is a little broader than I'd like it to be, in order to not give translators the chance to break functionality by accidentally omitting the .select-media class.

obenland added a commit that referenced this pull request Mar 17, 2017
@obenland obenland force-pushed the fix/delete-attachment branch from 30bfe96 to a5848c3 Compare March 17, 2017 20:13
obenland added a commit that referenced this pull request Mar 17, 2017
obenland added a commit that referenced this pull request Mar 17, 2017
obenland added a commit that referenced this pull request Mar 17, 2017
obenland added a commit that referenced this pull request Mar 17, 2017
obenland added a commit that referenced this pull request Mar 17, 2017
@obenland obenland force-pushed the fix/delete-attachment branch from 43e5c93 to 77735b2 Compare March 17, 2017 20:43
obenland added a commit that referenced this pull request Mar 17, 2017

if ( $use_count > 1 ) {
/* translators: %d is widget count */
$states[] = sprintf( __( 'Media Widgets (%d)' ), $use_count );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be changed to reference $this->l10n['media_library_state']?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'd want a singular and a plural version? Or just the singular?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plural conditional was an enhancement I came up with. Maybe it wouldn't be useful in practice, but I can imagine that it would be helpful for users to know how many widgets are using the image.

If we have both, then perhaps we'd need media_library_state_singular_widget and media_library_state_multiple_widgets

'change_media' => __( 'Change Media' ),
'edit_media' => __( 'Edit Media' ),
'error' => sprintf(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There could be additional errors in the future, so I think this should be attachment_missing_error instead.

@@ -73,6 +73,7 @@ wp.mediaWidgets = ( function( $ ) {
* @type {Object}
*/
events: {
'click .notice a': 'selectMedia',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the link should have a class name on it. If a subclass introduces a different error with a link in it, we'd want to make sure this event handler only handles clicks on the media library link. So either the container . notice element have a class like missing-attachment-error, or the link itself can have a media-library class that is then used in the selector here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I prefer the former, I'd like to avoid a situation where translators accidentally break functionality.

} );
attachment.fetch()
.done( function() {
control.selectedAttachment.set( attachment.attributes );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also ensure that the error flag is cleared for good measure?

control.selectedAttachment.set( _.extend( {}, attachment.attributes, { error: false } ) );

control.selectedAttachment.set( attachment.attributes );
} )
.fail( function() {
control.selectedAttachment.set( { error: true } );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the error here should be a code like missing_attachment instead of a boolean to anticipate additional error types in the future.

obenland added a commit that referenced this pull request Mar 17, 2017
@westonruter westonruter changed the title Delete widget with its attachment Handle deletion of attachment associated with media widget Mar 17, 2017
@obenland obenland force-pushed the fix/delete-attachment branch from 0b7def3 to b760046 Compare March 20, 2017 21:00
@obenland
Copy link
Contributor Author

Should have all your change requests addressed?

@westonruter
Copy link
Contributor

westonruter commented Mar 20, 2017

@obenland I saved an image widget that had an assigned attachment. I then deleted the underlying attachment and reloaded the customizer. When I did, I saw:

image

I would have expected there to be an error message. It seems the order of the conditionals wasn't right? I changed them in 7f32d28 and ac1239a, and now I see as expected:

image

@westonruter
Copy link
Contributor

Something to note is that the get-attachment admin-ajax request returns with just {"success":false} on a failure. When the request is resolved as rejected in Backbone, we don't know if the request failed due to the media having been deleted or due to there being a server error or even a connection failure (e.g. you just went into a tunnel).

esc_url( admin_url( 'upload.php' ) )
),
/* translators: %s is widget count */
'media_library_state' => _n_noop( 'Image Widget', 'Image Widget (%s)' ),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obenland PHP_CodeSniffer is complaining here:

Missing singular placeholder, needed for some languages. See https://codex.wordpress.org/I18n_for_WordPress_Developers#Plurals (WordPress.WP.I18n.MissingSingularPlaceholder)

Is this valid?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe these should actually be two separate strings.

For more about this, see WordPress/WordPress-Coding-Standards#567 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's a valid concern in our case. We're not saying One Widget, 2 Widgets, the label's singular/plural version is of no concern here really.

IMO we should rather skip the rule for that line than to introduce a language workaround (which in German at least reads quite awkwardly), what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #39.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants