WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 11 months ago

Last modified 11 months ago

#46866 closed enhancement (fixed)

Introduce the 'Uploaded to' onto the Full Media Editor

Reported by: garrett-eclipse Owned by: garrett-eclipse
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-screenshots commit
Focuses: ui, administration Cc:

Description

Hello,

When editing Media you'll find the 'Uploaded by'/'Uploaded to' information on the Media Modale Popup Editor. The Uploaded to is basically the parent information. However when you're viewing the FUll Media Editor it's not available. It would be nice to add 'Uploaded to' onto the Full Media Editor's publish metabox.

Thank you

Attachments (14)

Screen Shot 2019-04-09 at 12.23.03 PM.png (12.7 KB) - added by garrett-eclipse 2 years ago.
Uploaded to information on Media Modal editor
46866.1.patch (1.8 KB) - added by Mista-Flo 13 months ago.
Screenshot 2020-08-31 at 01.54.08.png (62.1 KB) - added by Mista-Flo 13 months ago.
Example with uploaded by
46866.2.diff (6.4 KB) - added by garrett-eclipse 13 months ago.
Updated patch to provide more consistency, icons and links to edit users.
Screen Shot 2020-08-31 at 10.56.44 AM.png (84.9 KB) - added by garrett-eclipse 13 months ago.
Full Attachment Editing Screen - Added Icons and linked 'Uploaded by'
Screen Shot 2020-08-31 at 10.57.11 AM.png (135.0 KB) - added by garrett-eclipse 13 months ago.
Updated Attachment Details Modal - Made consistent with ordering of meta, linked Uploaded By
46866.3.patch (6.5 KB) - added by Mista-Flo 12 months ago.
46866.4.patch (7.2 KB) - added by Mista-Flo 12 months ago.
Clean the code
Screen Shot 2019-04-09 at 12.22.53 PM.png (25.3 KB) - added by garrett-eclipse 11 months ago.
Full Media Editor missing Uploaded to information. Would be nice to display this just below the Uploaded on in the Save metabox [Note: this is a replacement image scrubbing the PII of the original.]
Screen Shot 2019-04-09 at 12.23.03 PM.2.png (9.7 KB) - added by garrett-eclipse 11 months ago.
Uploaded to information on Media Modal editor [Note: this is a replacement image scrubbing the PII of the original.]
46866.5.diff (7.2 KB) - added by garrett-eclipse 11 months ago.
Updated Patch to leave permission handling to the get_link functions. As well update author to provide no author when missing.
Screen Shot 2020-10-14 at 5.27.07 PM.png (61.7 KB) - added by garrett-eclipse 11 months ago.
Posts/Pages Publish modal are primarily icon based to illustrate the precedent.
46866.6.diff (7.2 KB) - added by garrett-eclipse 11 months ago.
Update the Uploaded To icon to a folder to indicate it's not the action but the location.
Screen Shot 2020-10-15 at 10.57.01 AM.png (81.4 KB) - added by garrett-eclipse 11 months ago.
Updated screen with folder icon.

Download all attachments as: .zip

Change History (50)

@garrett-eclipse
2 years ago

Uploaded to information on Media Modal editor

#1 @joemcgill
2 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

Hi @garrett-eclipse,

I think this could be a nice enhancement. Thanks for the suggestion.

@Mista-Flo
13 months ago

#2 @Mista-Flo
13 months ago

  • Keywords has-patch has-screenshots added; needs-patch removed

Hi, I have submitted a patch for the suggested changes.

@Mista-Flo
13 months ago

Example with uploaded by

#3 @Mista-Flo
13 months ago

@joemcgill Tell me if you have any suggestion, I wanted to reuse the same string 'Uploaded By' but it was a bit different with the uppercase on the b and the semi colon.

Do you think it's displayed in the good order? The file URL field seems dominant so it's a bit weird, but I thought it was the best place giving the fact we just printed the uploaded date before.

@garrett-eclipse
13 months ago

Updated patch to provide more consistency, icons and links to edit users.

@garrett-eclipse
13 months ago

Full Attachment Editing Screen - Added Icons and linked 'Uploaded by'

@garrett-eclipse
13 months ago

Updated Attachment Details Modal - Made consistent with ordering of meta, linked Uploaded By

#4 @garrett-eclipse
13 months ago

  • Keywords needs-testing needs-design-feedback added
  • Milestone changed from Future Release to 5.6

Thanks @Mista-Flo for getting this started, I've uploaded 46866.2.diff as another step as I found a few items we could improve on here while I tested your patch (which worked great btw).

  1. I've made the ordering of meta on the Attachment Details modal consistent with the full editor, this moves the Uploaded By & Uploaded To into the top right of the modal and allowed me to make their strings consistent.
  2. I've added icons to the Uploaded By & Uploaded To fields in the Full Attachment Editor to be consistent with Uploaded On.
  3. I've linked the Uploaded By field to the author profile. This I thought would be a nice bonus for admins allowing them to follow that breadcrumb if they find NSFW (etc.) images and need to go and ban the user responsible.

Would love your feedback/testing and I'm sure together we can get this into 5.6.

Marked needs-design-feedback to confirm the choice of dashicons and meta ordering as seen in the images above are appropriate.

#5 @Mista-Flo
13 months ago

Hey Garrett, thanks for your feedback and your patch, really nice enhancements :)

About the use of get_edit_user_link function, what will be shown to users that have access to edit attachment page but cannot edit user profiles data? A capability check would be nice I think except if you think WordPress will throw a WP_Error anyway and tell the user he is not allowed to view that page (I might be wrong).

#6 @garrett-eclipse
13 months ago

Thanks @Mista-Flo, to my understanding get_edit_user_link does the capability check for us;

if ( empty( $user_id ) || ! current_user_can( 'edit_user', $user_id ) ) {

Source - https://github.com/WordPress/wordpress-develop/blob/81a12a7bec6f82bb681176555308335822bd4010/src/wp-includes/link-template.php#L1625

The author was available already so the above check in get_edit_user_link means the link will only be made available if they have the edit_user capability.

That being said I do encourage testing on the capabilities to make sure we've not overlooked something.

And now looking at get_edit_post_link we can maybe save on the conditional in your original code, before grabbing the link from that function, as it already does a check on edit_post capability;
https://github.com/WordPress/wordpress-develop/blob/81a12a7bec6f82bb681176555308335822bd4010/src/wp-includes/link-template.php#L1378

This ticket was mentioned in Slack in #core-media by florian-tiar. View the logs.


13 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


12 months ago

This ticket was mentioned in Slack in #core-media by garrett-eclipse. View the logs.


12 months ago

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


12 months ago

@Mista-Flo
12 months ago

#11 @Mista-Flo
12 months ago

Hi there, just uploaded a new refreshed patch, just to avoid some PHP notice and extra unnecessary cap checks.

@Mista-Flo
12 months ago

Clean the code

This ticket was mentioned in Slack in #design by hareesh-pillai. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


11 months ago

#14 @hellofromTonya
11 months ago

This ticket needs design feedback to move it forward. @karmatosed Is this one on your radar?

#15 @karmatosed
11 months ago

@hellofromTonya thanks for raising this up, I'll make sure it gets a review this week.

#16 @hellofromTonya
11 months ago

Thank you @karmatosed.

Capturing notes from today's Media scrub on the status and next steps for this ticket:

Per @antpb:

We just need a thumbs up there, not really a design from the team so this doesnt seem like too big a lift to happen before Beta

I think this one is a crucial one for Media 5.6, it's such a nice visible enhancement

Once we get the thumbs up from design, this ticket can move forward to commit.

@garrett-eclipse
11 months ago

Full Media Editor missing Uploaded to information. Would be nice to display this just below the Uploaded on in the Save metabox [Note: this is a replacement image scrubbing the PII of the original.]

@garrett-eclipse
11 months ago

Uploaded to information on Media Modal editor [Note: this is a replacement image scrubbing the PII of the original.]

#17 @garrett-eclipse
11 months ago

You can ignore the two 'new' images, I noticed there was some PII on the original versions so scrubbed them. They were the original two from the ticket so wanted to preserve them just remove the PII.

#18 @garrett-eclipse
11 months ago

To avoid confusing design, below is the current iteration (not the two most recent images);
https://core.trac.wordpress.org/raw-attachment/ticket/46866/Screen%20Shot%202020-08-31%20at%2010.56.44%20AM.png

https://core.trac.wordpress.org/raw-attachment/ticket/46866/Screen%20Shot%202020-08-31%20at%2010.57.11%20AM.png

This ticket was mentioned in Slack in #core-media by garrett-eclipse. View the logs.


11 months ago

This ticket was mentioned in Slack in #core by francina. View the logs.


11 months ago

#21 follow-ups: @SergeyBiryukov
11 months ago

Just a quick note that the latest patch appears to remove some checks from wp_prepare_attachment_for_js():

  • $parent_type && $parent_type->show_ui
  • current_user_can( 'edit_post', $attachment->post_parent )

At a glance, these are still relevant to avoid displaying a link that should not be displayed if the parent post type doesn't have a UI or the current user cannot edit the parent post.

Would be good to test and verify that removing these does not lead to any unexpected results, e.g. displaying a link leading to nowhere, or PHP notices when the post type no longer exists.

#22 in reply to: ↑ 21 @Mista-Flo
11 months ago

Replying to SergeyBiryukov:

Just a quick note that the latest patch appears to remove some checks from wp_prepare_attachment_for_js():

  • $parent_type && $parent_type->show_ui
  • current_user_can( 'edit_post', $attachment->post_parent )

At a glance, these are still relevant to avoid displaying a link that should not be displayed if the parent post type doesn't have a UI or the current user cannot edit the parent post.

Would be good to test and verify that removing these does not lead to any unexpected results, e.g. displaying a link leading to nowhere, or PHP notices when the post type no longer exists.

Hi Sergey,

get_edit_post_link( $attachment->post_parent, 'raw' ); is actually doing capability check, returns an empty string if the user does not have the cap. Then in the template, we have the <# if ( data.uploadedToLink ) { #>.

So I think it was extra capability checks that are not useful anymore, we can clean the code, what do you think?

Last edited 11 months ago by Mista-Flo (previous) (diff)

@garrett-eclipse
11 months ago

Updated Patch to leave permission handling to the get_link functions. As well update author to provide no author when missing.

#23 in reply to: ↑ 21 @garrett-eclipse
11 months ago

Hello, I've updated the patch in 46866.5.diff and feel we're nearing the final iteration so please test and review away @Mista-Flo @antpb @SergeyBiryukov.

Replying to SergeyBiryukov:

Just a quick note that the latest patch appears to remove some checks from wp_prepare_attachment_for_js():

  • $parent_type && $parent_type->show_ui
  • current_user_can( 'edit_post', $attachment->post_parent )

At a glance, these are still relevant to avoid displaying a link that should not be displayed if the parent post type doesn't have a UI or the current user cannot edit the parent post.

Would be good to test and verify that removing these does not lead to any unexpected results, e.g. displaying a link leading to nowhere, or PHP notices when the post type no longer exists.

Yes as @Mista-Flo mentioned this was on purposed, after investigation all the checks for capabilities and show_ui is handled from within the get_link functions for us so we can remove them here and just rely on the empty link check to tell if we can insert the link or not.

In get_edit_user_link the following check is done for us;
current_user_can_edit_user - https://github.com/WordPress/wordpress-develop/blob/79703088c4fea9655b5f071cadee4a7949dd1cca/src/wp-includes/link-template.php#L1627

In get_edit_post_link the following checks are done for us;
current_user_can_edit_post - https://github.com/WordPress/wordpress-develop/blob/79703088c4fea9655b5f071cadee4a7949dd1cca/src/wp-includes/link-template.php#L1380
post_type_object_has_edit_link - https://github.com/WordPress/wordpress-develop/blob/79703088c4fea9655b5f071cadee4a7949dd1cca/src/wp-includes/link-template.php#L1380
In this function there's an _edit_link check, and in the constructor for post_types that will be empty if show_ui or has_edit_link are empty/false - https://github.com/WordPress/wordpress-develop/blob/f36c42c70034348c4007157c1fc8a8d69e50c90c/src/wp-includes/class-wp-post-type.php#L469

So to summarize, all checks are done for us. There was two 'read' checks but from our discussion on slack here they were removed to match with the author setup and avoid admins seeing a attachment as an orphan when it isn't.

Also in this update I use the author exist check to provide no author verbiage if they don't similar to what was found for the media template.

I do encourage thorough testing of the capabilities and review.

This ticket was mentioned in Slack in #core by garrett-eclipse. View the logs.


11 months ago

This ticket was mentioned in Slack in #core-media by garrett-eclipse. View the logs.


11 months ago

#26 @garrett-eclipse
11 months ago

  • Owner set to garrett-eclipse
  • Status changed from new to accepted

I'm going to own the ticket so it appears in the links I've been sharing requesting testing.

This ticket was mentioned in Slack in #design by garrett-eclipse. View the logs.


11 months ago

#28 follow-up: @karmatosed
11 months ago

This is quite a long ticket so let me try and work through this with some feedback.

My initial thought is this section is getting informationally dense, but that's something to maybe look to iterate and shouldn't be something we judge this usefulness on.

I do however wonder if we need the icon. I guess if we have a president that is ok, but I'd be tempted to explore even removing those. I just would lean to no icon, particularly as that one means you would click to upload.

Beyond that, I think working within the boundaries you have, this goes with that. I truly would love to iterate this section though, but that's beyond the scope of this addition.

#29 @karmatosed
11 months ago

  • Keywords needs-design-feedback removed

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


11 months ago

#31 @hellofromTonya
11 months ago

  • Keywords commit added

Moving this ticket to commit after reviewing with @antpb and @garrett-eclipse.

@garrett-eclipse
11 months ago

Posts/Pages Publish modal are primarily icon based to illustrate the precedent.

@garrett-eclipse
11 months ago

Update the Uploaded To icon to a folder to indicate it's not the action but the location.

@garrett-eclipse
11 months ago

Updated screen with folder icon.

#32 in reply to: ↑ 28 @garrett-eclipse
11 months ago

  • Keywords needs-testing removed

Thank you for the feedback @karmatosed :

This is quite a long ticket so let me try and work through this with some feedback.

My initial thought is this section is getting informationally dense, but that's something to maybe look to iterate and shouldn't be something we judge this usefulness on.

Agreed it's starting to get heavy, I'm hoping that's the extent we'll ever get to in core otherwise would start moving the file specific meta somewhere less prominent.

I do however wonder if we need the icon. I guess if we have a president that is ok, but I'd be tempted to explore even removing those. I just would lean to no icon, particularly as that one means you would click to upload.

I think we should look into the removal of icons in future as the precedent comes from the conformity to the other editor screens (Posts, Pages) which I'll attach. We can revisit for all of these screens if you feel there's merit.
As to the upload icon, that makes sense, I've uploaded 46866.6.diff to switch it out for the folder icon which represents storage location more. We could also go with the post/page icon instead or am open to your thoughts on options. We should us dashicons, the one I'm using now - https://developer.wordpress.org/resource/dashicons/#category

Beyond that, I think working within the boundaries you have, this goes with that. I truly would love to iterate this section though, but that's beyond the scope of this addition.

Sounds good, let's try to connect in a design meeting once 5.7 has started up.

#33 @karmatosed
11 months ago

@garrett-eclipse much prefer this icon. I do think there is merit in the future exploring no icons, but that's not for today. Thanks for taking the feedback and iterating.

This ticket was mentioned in Slack in #core-media by garrett-eclipse. View the logs.


11 months ago

#35 @antpb
11 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 49207:

Media: Add 'Uploaded to' for individual media items in the media editor.
Adds a link in the media editor showing which post a media item was uploaded to.
Props karmatosed, garrett-eclipse, Mista-Flo, SergeyBiryukov, joemcgill, hellofromTonya.
Fixes #46866.

#36 @SergeyBiryukov
11 months ago

In 49209:

Coding Standards: Fix WPCS issues in [49207].

See #46866.

Note: See TracTickets for help on using tickets.