WordPress.org

Make WordPress Core

Opened 19 months ago

Closed 8 days ago

Last modified 8 days 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 19 months ago.
Uploaded to information on Media Modal editor
46866.1.patch (1.8 KB) - added by Mista-Flo 2 months ago.
Screenshot 2020-08-31 at 01.54.08.png (62.1 KB) - added by Mista-Flo 2 months ago.
Example with uploaded by
46866.2.diff (6.4 KB) - added by garrett-eclipse 2 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 2 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 2 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 4 weeks ago.
46866.4.patch (7.2 KB) - added by Mista-Flo 4 weeks ago.
Clean the code
Screen Shot 2019-04-09 at 12.22.53 PM.png (25.3 KB) - added by garrett-eclipse 3 weeks 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 3 weeks 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 3 weeks 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 12 days ago.
Posts/Pages Publish modal are primarily icon based to illustrate the precedent.
46866.6.diff (7.2 KB) - added by garrett-eclipse 12 days 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 12 days ago.
Updated screen with folder icon.

Download all attachments as: .zip

Change History (50)

@garrett-eclipse
19 months ago

Uploaded to information on Media Modal editor

#1 @joemcgill
19 months 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
2 months ago

#2 @Mista-Flo
2 months ago

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

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

@Mista-Flo
2 months ago

Example with uploaded by

#3 @Mista-Flo
2 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
2 months ago

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

@garrett-eclipse
2 months ago

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

@garrett-eclipse
2 months ago

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

#4 @garrett-eclipse
2 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
2 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
8 weeks 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.


8 weeks ago

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


7 weeks ago

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


4 weeks ago

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


4 weeks ago

@Mista-Flo
4 weeks ago

#11 @Mista-Flo
4 weeks ago

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

@Mista-Flo
4 weeks ago

Clean the code

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


3 weeks ago

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


3 weeks ago

#14 @hellofromTonya
3 weeks ago

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

#15 @karmatosed
3 weeks ago

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

#16 @hellofromTonya
3 weeks 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
3 weeks 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
3 weeks ago

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

#17 @garrett-eclipse
3 weeks 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
3 weeks 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.


3 weeks ago

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


3 weeks ago

#21 follow-ups: @SergeyBiryukov
3 weeks 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
3 weeks 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 3 weeks ago by Mista-Flo (previous) (diff)

@garrett-eclipse
3 weeks 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
3 weeks 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.


3 weeks ago

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


3 weeks ago

#26 @garrett-eclipse
3 weeks 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.


13 days ago

#28 follow-up: @karmatosed
13 days 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
13 days ago

  • Keywords needs-design-feedback removed

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


12 days ago

#31 @hellofromTonya
12 days ago

  • Keywords commit added

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

@garrett-eclipse
12 days ago

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

@garrett-eclipse
12 days ago

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

@garrett-eclipse
12 days ago

Updated screen with folder icon.

#32 in reply to: ↑ 28 @garrett-eclipse
12 days 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
12 days 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.


8 days ago

#35 @antpb
8 days 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
8 days ago

In 49209:

Coding Standards: Fix WPCS issues in [49207].

See #46866.

Note: See TracTickets for help on using tickets.