Total number of attachments not updated when attachment uploaded

[joedolson]

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.

[desrosj]

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.

[joedolson]

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.

[antpb]

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]

First pass

[joedolson]

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.

Trac ticket: https://core.trac.wordpress.org/ticket/53171

[adamsilverstein]

@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.

[adamsilverstein]

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

[joedolson]

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!

[joedolson]

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

[joedolson]

Updated docs & arguments

[joedolson]

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

[joedolson]

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.