Page MenuHomePhabricator

[Regression] "Copy metadata" link not always at the 1st image
Closed, ResolvedPublic

Description

The "Copy metadata" functionality is lost after the code update on 30. August 2012.


Version: master
Severity: major

Details

Reference
bz39852

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 12:54 AM
bzimport added a project: UploadWizard.
bzimport set Reference to bz39852.
bzimport added a subscriber: Unknown Object (MLST).

Rephrase:

I found the link now in a batch upoad of 16 images: The "copy metadata" link is now at the 4th image of the batch. I guess the 4th postion is random?

I don't think this is a regression, or at least, I think the regression is caused by us assuming reasonable treatment of upload order. See also bug 39746.

It's a regression in that the upload order is now broken again, causing this functionality to fail as well.

See also https://gerrit.wikimedia.org/r/#/c/4987/ which was the change I made a while ago to fix the display order.

Erik,

How hard would it be to properly handle array deletions and things inside of each upload? We basically need to make sure that the reservedIndex members are sequential, at the very least by the end of the selection process....or maybe at the beginning of startUploads?

There is still an unprotect access to uploads[0] on line 761 of mw.UploadWizard.js

22273 doesn't fix the problem, unfortunately. The problem is caused by the call to showNext() that was added to mw.UploadWizardUpload.js here:
https://gerrit.wikimedia.org/r/#/c/20814/13/resources/mw.UploadWizardUpload.js

Unfortunately, removing that call breaks (in certain cases) the new warning overriding feature that was added.

The basic problem is that the ordering of the images (and how they are added into the uploads array) has been complicated by the fact that we now allow users to override uploads that fail due to warnings. Because of this feature, we don't actually know what images are going to get used until the user clicks the Continue button and advances to the deeds step. There are a few different ways this can be fixed.

  1. Quick and dirty - disable warning overriding until we can rejigger the relevant pieces.
  1. A bit complicated - Rejigger things now. Mainly we need to un-overload the showNext() function. This function is being used for several different purposes now, and some of them conflict with each other. We should move the appending functionality and the copyMetadata functionality out of showNext() and instead trigger them when the user advances to the deeds step (see moveToStep). Secondly, we shouldn't hard-code copyMetadata to uploads[0]. There are several cases where this will fail. Instead, once the user had advanced to the deeds step (or the details step), show the copyMetadataCtrlDiv for whichever upload is currently first in the list (this might require a little retooling of the copyMetadata functions as well).
  1. There are a few hackish (but lower risk) ways we could patch things up without significantly changing how showNext works. I haven't totally thought these through, but one way would be to build a custom success handler for the case where a warning is overridden and remove the showNext call from the regular setSuccess method. This wouldn't solve all the issues, but it would probably solve most of the common scenarios.

If we can safely remove the warning override option (we only trigger it for duplicate-archive cases, right?), I'd suggest we do that for now so we can fix the regression and then implement a proper fix later.

Yes, it's only for duplicate-archives cases currently. If we remove the override option, we can also safely remove the showNext call from the setSuccess method, which should eliminate the inconsistent ordering.

I'm always for fixing things now, but if we only have time for the quick option, I'd prefer number 1 to number 3 I think. Thanks, kaldari.

https://gerrit.wikimedia.org/r/#/c/65318/ attempts to re-enable the option to upload previously deleted files

Gilles triaged this task as Unbreak Now! priority.Dec 4 2014, 10:11 AM
Gilles moved this task from Untriaged to Done on the Multimedia board.
Gilles lowered the priority of this task from Unbreak Now! to Needs Triage.Dec 4 2014, 11:22 AM