WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

#53171 closed defect (bug) (fixed)

Total number of attachments not updated when attachment uploaded

Reported by: joedolson Owned by: joedolson
Milestone: 5.8 Priority: normal
Severity: normal Version: 5.8
Component: Media Keywords: has-patch commit
Focuses: javascript, administration Cc:

Description

With the removal of infinite scroll in #50105, we're now displaying a value for the total number of attachments as part of the 'load more' interface. This value doesn't update if an attachment is added or removed from the visible collection.

It looks like the the total attachments are only set when there's a collection query, and this isn't run when an item is uploaded or deleted. See the parse method in media/models/attachments.js, where this.totalAttachments is defined.

Attachments (3)

53171.diff (2.1 KB) - added by joedolson 6 months ago.
First pass
53171.3.diff (1.9 KB) - added by adamsilverstein 6 months ago.
53171.4.diff (1.4 KB) - added by joedolson 6 months ago.
Updated docs & arguments

Download all attachments as: .zip

Change History (19)

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


7 months ago

#2 @desrosj
6 months ago

  • Keywords needs-patch added

This one still needs a patch. Today is the beta 1 deadline, but I think this one can remain as it directly relates to a feature/enhancement being shipped in 5.8.

@joedolson do you think this is realistic to include in 5.8? Ideally this would be fixed before beta 3 on 22 June. But absolutely must be fixed by 29 June to be included.

#3 @joedolson
6 months ago

I think it's feasible, assuming we work out a change to how the number of attachments is passed into BackBone in #50105, which is currently in progress. But it depends on solving that problem, so there definitely won't be a patch for this before that's solved.

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


6 months ago

#5 @antpb
6 months ago

We are leaving this open through Beta 1 as this is partially merged or related to a partially merged feature. All that is needed is a review of the above patch and potentially refinement. If the patch can't be merged before Beta 3 we'll need to revert to allow time for the revert to be tested before Release Candidate.

@joedolson
6 months ago

First pass

#6 @joedolson
6 months ago

  • Keywords has-patch needs-testing 2nd-opinion added; needs-patch removed

I'm not sure this is the best way to handle this, but it does work. Patch adds two private functions attached to the observer on the attachments model, one tracking additions and one on removals. It just increments the total attachments up or down one based on behavior.

Tested when adding an attachment, deleting an attachment, bulk deleting attachments, and bulk adding items.

What I'm unsure about is whether incrementing up and down is really going to be sufficiently reliable - however, we aren't updating the query results on add/remove events, so getting that from the AJAX response doesn't seem viable.

I tested this with 50105.13.diff applied from #50105, but I don't think anything in this patch actually depends on that.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


6 months ago

#8 @joedolson
6 months ago

  • Owner set to joedolson
  • Status changed from assigned to accepted

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


6 months ago

#11 @adamsilverstein
6 months ago

@joedolson Adding handlers for add and remove does seem like a very standard way of syncing collection state in Backbone apps. I tested the patch adding and removing single and multiple images and the image count in the footer always stayed synced with the collection.

I did notice one issue: a couple of my image uploads failed (images were too large). In the console, I see an error "Uncaught TypeError: Cannot read property 'totalAttachments' of undefined" on L643, the call to this.mirroring.totalAttachments = this.mirroring.totalAttachments - 1

So I think we need to add a conditional in _removeFromTotalAttachments like you have in the add handler.

#12 @adamsilverstein
6 months ago

In 53171.3.diff adds a check in _removeFromTotalAttachments to ensure this.mirroring is set before using it. Fixes the previously mentioned issue.

Last edited 6 months ago by adamsilverstein (previous) (diff)

#13 @joedolson
6 months ago

  • Keywords 2nd-opinion removed

That's a good catch! I hadn't tested error conditions, so I'd only seen that an error was thrown when adding attachments.

If you think this is a reasonably standard way of handling this, I'm satisfied - I'm not super comfortable with backbone, so a second opinion is great. Thanks!

#14 @joedolson
6 months ago

  • Keywords commit added; needs-testing removed

Tested the updated patch; looks good. Going to go ahead and get this committed.

@joedolson
6 months ago

Updated docs & arguments

#15 @joedolson
6 months ago

Final patch. Updates function docs and eliminates unused function arguments.

#16 @joedolson
6 months ago

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

In 51191:

Media: Update total attachment count when media added or removed.

Add handlers to increment the total attachments count for the media collection when an item is added or removed.

props adamsilverstein.
Fixes #53171.

Note: See TracTickets for help on using tickets.