r87031 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r87030‎ | r87031 | r87032 >
Date:19:13, 27 April 2011
Author:kaldari
Status:resolved (Comments)
Tags:
Comment:
further work on FlickrChecker module
Modified paths:
  • /trunk/extensions/UploadWizard/UploadWizard.i18n.php (modified) (history)
  • /trunk/extensions/UploadWizard/UploadWizardHooks.php (modified) (history)
  • /trunk/extensions/UploadWizard/resources/mw.FlickrChecker.js (modified) (history)

Diff [purge]

Index: trunk/extensions/UploadWizard/UploadWizardHooks.php
@@ -59,7 +59,8 @@
6060 'resources/mw.IframeTransport.js',
6161 'resources/mw.ApiUploadHandler.js',
6262 'resources/mw.DestinationChecker.js',
63 - 'resources/mw.UploadWizardUtil.js',
 63+ 'resources/mw.UploadWizardUtil.js',
 64+ 'resources/mw.FlickrChecker.js',
6465
6566 // interface libraries
6667 'resources/mw.GroupProgressBar.js',
@@ -309,6 +310,8 @@
310311 'mwe-upwiz-license-none-applicable',
311312 'mwe-upwiz-license-confirm-remove',
312313 'mwe-upwiz-license-confirm-remove-title',
 314+ 'mwe-upwiz-license-external',
 315+ 'mwe-upwiz-license-external-invalid',
313316 'mwe-upwiz-categories',
314317 'mwe-upwiz-categories-add',
315318 'mwe-upwiz-category-remove',
Index: trunk/extensions/UploadWizard/UploadWizard.i18n.php
@@ -258,6 +258,9 @@
259259 'mwe-upwiz-license-none-applicable' => 'Abandon {{PLURAL:$1|this upload|these uploads}} without publishing',
260260 'mwe-upwiz-license-confirm-remove' => 'Are you sure you want to remove {{PLURAL:$1|this upload|these uploads}}?',
261261 'mwe-upwiz-license-confirm-remove-title' => 'Confirm remove',
 262+
 263+ 'mwe-upwiz-license-external' => 'This file is under the following license on $1: <b>$2</b>.',
 264+ 'mwe-upwiz-license-external-invalid' => 'This file is under the following license on $1: <b>$2</b>. Unfortunately, this license is not appropriate for use on {{SITENAME}}.',
262265
263266
264267 'mwe-upwiz-categories' => 'Categories',
Index: trunk/extensions/UploadWizard/resources/mw.FlickrChecker.js
@@ -8,14 +8,15 @@
99
1010 // Map each Flickr license name to the equivalent templates.
1111 // These are the current Flickr license names as of April 26, 2011.
 12+ // Live list at http://api.flickr.com/services/rest/?&method=flickr.photos.licenses.getInfo&api_key=e9d8174a79c782745289969a45d350e8
1213 licenseMaps: {
1314 'All Rights Reserved': 'invalid',
 15+ 'Attribution License': 'invalid',
 16+ 'Attribution-NoDerivs License': 'invalid',
 17+ 'Attribution-NonCommercial-NoDerivs License': 'invalid',
 18+ 'Attribution-NonCommercial License': 'invalid',
1419 'Attribution-NonCommercial-ShareAlike License': 'invalid',
15 - 'Attribution-NonCommercial License': 'invalid',
16 - 'Attribution-NonCommercial-NoDerivs License': 'invalid',
17 - 'Attribution License': '{{flickrreview}}{{cc-by-2.0}}',
1820 'Attribution-ShareAlike License': '{{flickrreview}}{{cc-by-sa-2.0}}',
19 - 'Attribution-NoDerivs License': 'invalid',
2021 'No known copyright restrictions': '{{flickrreview}}{{Flickr-no known copyright restrictions}}',
2122 'United States Government Work': '{{flickrreview}}{{PD-USGov}}'
2223 },
@@ -24,7 +25,9 @@
2526 * If a photo is from flickr, retrieve its license. If the license is valid, display the license
2627 * to the user, hide the normal license selection interface, and set it as the deed for the upload.
2728 * If the license is not valid, alert the user with an error message. If no recognized license is
28 - * retrieved, do nothing.
 29+ * retrieved, do nothing. Note that the license look-up system is fragile on purpose. If Flickr
 30+ * changes the name associated with a license ID, it's better for the lookup to fail than to use
 31+ * an incorrect license.
2932 * @param url - the source URL to check
3033 * @param $selector - the element to insert the license name into
3134 * @param upload - the upload object to set the deed for
@@ -36,9 +39,16 @@
3740 $.getJSON(this.apiUrl+'&method=flickr.photos.getInfo&api_key='+this.apiKey+'&photo_id='+photoId+'&format=json&jsoncallback=?',
3841 function( data ) {
3942 if ( typeof data.photo != 'undefined' ) {
40 - // XXX do all the work here
4143 // The returned data.photo.license is just an ID that we use to look up the license name
42 - console.debug( mw.FlickrChecker.licenseList[data.photo.license] );
 44+ var licenseName = mw.FlickrChecker.licenseList[data.photo.license];
 45+ // Use the license name to retrieve the template values
 46+ var licenseValue = mw.FlickrChecker.licenseMaps[licenseName];
 47+ // Set the license message to show the user.
 48+ if ( licenseValue == 'invalid' ) {
 49+ var licenseMessage = gM( 'mwe-upwiz-license-external-invalid', 'Flickr', licenseName );
 50+ } else {
 51+ var licenseMessage = gM( 'mwe-upwiz-license-external', 'Flickr', licenseName );
 52+ }
4353 }
4454 }
4555 );

Follow-up revisions

RevisionCommit summaryAuthorDate
r87258FlickrChecker fixes, follow-up to r87002 and r87031kaldari17:24, 2 May 2011

Comments

#Comment by NeilK (talk | contribs)   20:17, 27 April 2011

ok if fixmes to previous revs are fixed

#Comment by Catrope (talk | contribs)   11:27, 2 May 2011
+							var licenseMessage = gM( 'mwe-upwiz-license-external-invalid', 'Flickr', licenseName );

There is no return statement or action in checkFlickr(), the return value is set to a local variable and discarded immediately. AFAICT this function doesn't actually do anything with the information it retrieves, surely that's wrong?

#Comment by Kaldari (talk | contribs)   17:29, 2 May 2011

I wanted to get feedback from neil on the interface implementation before I actually did anything with the data. You're correct that right now it doesn't do anything. I added a comment to that effect in r87258, so that it's not confusing.

#Comment by Catrope (talk | contribs)   12:59, 2 May 2011
+	 * Retrieve the list of all current Flickr licenses and store it in an array (mw.FlickrChecker.licenses)
[...]
+						mw.FlickrChecker.licenseList[value.id] = value.name;

Documentation doesn't match the actual behavior.

Also, this file aliases mw to mediaWiki correctly with a closure, but doesn't do the same with $.

#Comment by Kaldari (talk | contribs)   17:29, 2 May 2011

Fixed in r87258.

Status & tagging log