Last modified: 2011-10-27 00:43:47 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T31346, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 29346 - Inaccurately tagging some images as being from Flickr and requiring review and multiple template-loops by altering the configuration of Upload Wizard
Inaccurately tagging some images as being from Flickr and requiring review an...
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
UploadWizard (Other open bugs)
unspecified
All All
: Highest normal (vote)
: ---
Assigned To: Neil Kandalgaonkar
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-11 09:05 UTC by Rainer Rillke @commons.wikimedia
Modified: 2011-10-27 00:43 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
A few comments on mw.UploadWizardLicenseInput.js (9.26 KB, application/x-javascript)
2011-06-11 09:05 UTC, Rainer Rillke @commons.wikimedia
Details
screenshot of firebug before modification took place (40.94 KB, image/png)
2011-06-11 09:07 UTC, Rainer Rillke @commons.wikimedia
Details
screenshot of firebug @ modification time (58.98 KB, image/png)
2011-06-11 09:08 UTC, Rainer Rillke @commons.wikimedia
Details
screenshot of firebug after modification took place (41.65 KB, image/png)
2011-06-11 09:09 UTC, Rainer Rillke @commons.wikimedia
Details
Common's configuration (5.93 KB, text/plain)
2011-06-11 09:16 UTC, Rainer Rillke @commons.wikimedia
Details
requested bugfix (9.31 KB, patch)
2011-06-18 13:43 UTC, Rainer Rillke @commons.wikimedia
Details
patch in unified diff - format as requested (1.02 KB, patch)
2011-07-31 09:38 UTC, Rainer Rillke @commons.wikimedia
Details

Description Rainer Rillke @commons.wikimedia 2011-06-11 09:05:16 UTC
Created attachment 8637 [details]
A few comments on mw.UploadWizardLicenseInput.js

Symptoms:

http://commons.wikimedia.org/wiki/Commons:Prototype_upload_wizard_feedback#Flickr_template_added.2C_while_there_is_no_link_to_Flickr

Source:

http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/UploadWizard/resources/mw.UploadWizardLicenseInput.js?revision=88694&view=markup


---------------------------------------------

I think objects are assigned by reference. I think It's not a good idea to change the configuration. See attachment.
Comment 1 Rainer Rillke @commons.wikimedia 2011-06-11 09:07:47 UTC
Created attachment 8638 [details]
screenshot of firebug before modification took place
Comment 2 Rainer Rillke @commons.wikimedia 2011-06-11 09:08:52 UTC
Created attachment 8639 [details]
screenshot of firebug @ modification time
Comment 3 Rainer Rillke @commons.wikimedia 2011-06-11 09:09:21 UTC
Created attachment 8640 [details]
screenshot of firebug after modification took place
Comment 4 Rainer Rillke @commons.wikimedia 2011-06-11 09:16:39 UTC
Created attachment 8641 [details]
Common's configuration
Comment 5 Roan Kattouw 2011-06-11 12:06:12 UTC
(In reply to comment #0)
> Created attachment 8637 [details]
> A few comments on mw.UploadWizardLicenseInput.js
>
Instead of re-submitting the entire file, please submit a patch file instead.
Comment 6 Rainer Rillke @commons.wikimedia 2011-06-11 13:17:38 UTC
Sorry for being not a professional developer. I was not involved in the development of upload wizard and simply wanted to submit a bug. I am not familiar with the deep structure of it.

But I suggest replacing l. 61 which currently is
var templates = mw.isDefined( license.props['templates'] ) ? license.props.templates : [ license.name ];

with

var templates = mw.isDefined( license.props['templates'] ) ? $j.extend(true, {}, license.props.templates) : [ license.name ];

or something similar. I don't know the installation of jQuery and so on ... It is your turn to find out whether this would work.

Sincerely Rillke
Comment 7 Neil Kandalgaonkar 2011-06-11 18:46:22 UTC
Rainer: I don't know what problem you are trying to solve here and don't have the time to figure it out. Please explain it in words.
Comment 8 Neil Kandalgaonkar 2011-06-11 18:50:43 UTC
(In reply to comment #6)
>  It
> is your turn to find out whether this would work.

I appreciate your enthusiasm but are you *sure* you can't figure out if your patch works or not? I would happily commit a well-tested patch.

Otherwise this goes to the bottom of my "things to do" pile.
Comment 9 Neil Kandalgaonkar 2011-06-13 16:00:44 UTC
Marking as enhancement unless / until this patch is shown to fix an existing bug, or explained in some way.
Comment 10 fvanlamoen 2011-06-15 08:00:18 UTC
The problem is that when an image is uploaded using UploadWizard on commons that is licensed with {{PD-USGov}}, then it seems that automaticly a {{flickrreview}} is attached, while there is not the slightest relation to flickr. Here is an example: http://commons.wikimedia.org/w/index.php?title=File:Bellamy_House_Wilmington_North_Carolina_by_Frances_Benjamin_Johnston.jpg&oldid=55506689.
Comment 11 Rainer Rillke @commons.wikimedia 2011-06-18 13:43:29 UTC
Created attachment 8677 [details]
requested bugfix

The change of the bug-title was wrong. This is only the symptom.

I now submit a fix that should work. 

Again: The script changed the media-wiki-config, set with (mediaWiki.config.set({"UploadWizardConfig": {) (see previous attachment). This is now prevented by cloning the array. Don't presume I am talking about ghosts again. That's something I really dislike. If you would have taken the time to read my comments in the "resubmitted source-file", you would know what I am talking about. Grrrrr ;-)

With best regards Rainer Rillke (RE)
Comment 12 Roan Kattouw 2011-06-18 20:23:47 UTC
(In reply to comment #9)
> Marking as enhancement unless / until this patch is shown to fix an existing
> bug, or explained in some way.
Neil, this looks legit to me. Apparently some config var is assigned to another var somewhere and manipulated later, but because of the by-reference nature of array assignments in JS, this ends up manipulating the config itself, which is kind of evil and apparently leads to pollution. The proposed fix (there's no patch but see comment #6) is to clone the array with something like var bar = $.extend( {}, foo ); instead of var bar = foo;
Comment 13 Rainer Rillke @commons.wikimedia 2011-06-19 09:04:41 UTC
> there's no patch but see comment #6
Now, there is a patch, feel free to remove the comments in ll. 59, 61, 65
> $.extend( {}, foo );
Please do not use this. Instead use $.extend( [], foo ); Otherwise it is not an array, only an object (without length property and join-function, ...)
But you can just use my proposed fix in comment #11

Thanks for working on this subject. Sincerely Rillke
Comment 14 Roan Kattouw 2011-06-19 12:47:22 UTC
(In reply to comment #13)
> > there's no patch but see comment #6
> Now, there is a patch, feel free to remove the comments in ll. 59, 61, 65
By 'patch' I meant a patch file in unified diff format, listing the changes between our version and yours.

> > $.extend( {}, foo );
> Please do not use this. Instead use $.extend( [], foo ); Otherwise it is not an
> array, only an object (without length property and join-function, ...)
> But you can just use my proposed fix in comment #11
>
Yes, [] instead of {}, my bad.
Comment 15 Rainer Rillke @commons.wikimedia 2011-06-19 15:18:48 UTC
> unified diff format
 --> is this something I can do?

Help says: "If the patch is not in a format that you like, you can turn it into a unified diff format by clicking the "Raw Unified" link at the top of the page."

Should I have submitted the original file first and then the changes? (Sorry I'm new to bugzilla.)

Is there another way to speed-up fixing?
Comment 16 Roan Kattouw 2011-06-19 19:23:51 UTC
(In reply to comment #15)
> > unified diff format
>  --> is this something I can do?
> 
> Help says: "If the patch is not in a format that you like, you can turn it into
> a unified diff format by clicking the "Raw Unified" link at the top of the
> page."
> 
> Should I have submitted the original file first and then the changes? (Sorry
> I'm new to bugzilla.)
> 
I have no idea how that works. I just make my patches on the command line.

> Is there another way to speed-up fixing?
It's a one-line fix so this is fine. For now you'll just have to wait until Monday or later for Neil to see this and have time for this.
Comment 17 Rainer Rillke @commons.wikimedia 2011-07-21 09:01:39 UTC
And now we have a new Bug 29994 . With some additions. 

But the license templates are just a configuration issue. You just have to change licensesThirdParty of comment #4 from "and" to "or" . That's all.
Comment 18 Rainer Rillke @commons.wikimedia 2011-07-31 09:38:47 UTC
Created attachment 8855 [details]
patch in unified diff - format as requested

This patch should resolve the following symptoms:
* Inaccurately adding License-Review templates if user selected PD-US-Gov
* Prevent Template-Loop like {{self|self|self|cc-0}}

This is done by cloning the object instead of passing a reference.

Q: Why do you use a deep-copy?
A: We do not know what follows in future so a deep copy is much saver.

WARNING: There is no warranty of merchantability nor of fitness for a particular purpose.
Comment 19 Ryan Kaldari 2011-08-18 23:50:29 UTC
This could also be fixed by integrating the mw.FlickrChecker.js functionality into UploadWizard, so that Flickr licenses are handled automatically. The code that interfaces with Flickr is already written, but I never actually integrated it into UW.
Comment 20 Brion Vibber 2011-09-22 01:01:12 UTC
Is this still an issue? I can't reproduce it locally on trunk; if I check "Original work of the US Federal Government", I only get a {{PD-USGov}}, not a flickr-review tag.

... I can however reproduce it on Commons, doing the same thing: http://commons.wikimedia.org/wiki/File:Test_file_for_bug_29346.png
Comment 21 Neil Kandalgaonkar 2011-09-22 01:28:33 UTC
The author of the bug report wanted to fix multiple issues. The one that's more of a policy change was split off into #29994, which is good.

The rest is the same underlying issue as #30237, which I just fixed. In retrospect, now I understand what Rainier was talking about.

*** This bug has been marked as a duplicate of bug 30237 ***
Comment 22 Rainer Rillke @commons.wikimedia 2011-09-22 13:59:17 UTC
Is it common practise to mark older bugs as dupes of newer ones, Neil?
Comment 23 Neil Kandalgaonkar 2011-09-22 17:26:10 UTC
(In reply to comment #22)
> Is it common practise to mark older bugs as dupes of newer ones, Neil?

I am from the future.
Comment 24 Rainer Rillke @commons.wikimedia 2011-10-14 11:43:32 UTC
Sorry, I am not (from the future). And someone seem to forgot to deploy this to MW1.18. Ouch! Just try upload wizard on commons. It produces the same mistakes again:
*self-loop: 
1) upload multiple files and "select the license for each file in the next step", then select cc0
2) cancel the upload declaring it is not your own work and then selecting a file and uploading this
* flickrreview:
1) at us-gov-source not from flickr

Someone from the past could be upset now. ;-)
Comment 25 Neil Kandalgaonkar 2011-10-14 12:01:08 UTC
Rainer: I feel your pain. After we deployed I saw that again and was hoping it was some kind of optical illusion.

We are deploying again Monday, hopefully. This time I will rebranch from trunk which ought to cure all regressions.
Comment 26 Mark A. Hershberger 2011-10-26 00:00:25 UTC
(In reply to comment #25)
> We are deploying again Monday, hopefully. This time I will rebranch from trunk
> which ought to cure all regressions.

Did it?

Please close this bug if they are cured.
Comment 27 Neil Kandalgaonkar 2011-10-27 00:43:47 UTC
after the deploy on 2011-10-26 this should be fixed again. Tested and cannot make it happen.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links