Launchpad itself

Merge lp:~adiroiban/launchpad/bug-402235 into lp:launchpad

  1. bug-402235
  2. Merge into devel
Proposed by Adi Roiban
Status: Merged
Merged at revision: 10871
Proposed branch: lp:~adiroiban/launchpad/bug-402235
Merge into: lp:launchpad
Diff against target: 381 lines (+239/-18)
3 files modified
lib/canonical/launchpad/javascript/translations/pofile.js (+88/-15)
lib/lp/translations/templates/currenttranslationmessage-translate-one.pt (+2/-1)
lib/lp/translations/windmill/tests/test_pofile_translate.py (+149/-2)
To merge this branch: bzr merge lp:~adiroiban/launchpad/bug-402235
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Review via email: mp+23973@code.launchpad.net

Commit message

Make diverge and force suggestion checkboxes mutually exclusive.

Description of the change

= Bug 402235 =

In the view for a TranslationMessage ("zoomed-in"), both options "Make this translation specific to" and "Someone should review this translation" can be selected although they are mutually exclusive. Doing so currently does no harm but simply ignores the diverged option in favour of the suggestion option.

== Proposed fix ==

Make "Make this translation specific to" and "Someone should review this translation" mutually exclusive ... checking one will disable the other. Un-checking will enable the other.

There is an exception: when all translations are marked for dismissal, un-checking the diverge checkbox will keep the "force suggestion" checkbox disabled.

== Pre-implementation notes ==

None yet.

== Implementation details ==

I have also fixed a small bug in the setupSuggestionDismissal to make sure the disabled inputs are not selected.

The progressive Javascript initialization was refactored in the fix for bug 201749, but Henning was a bit busy and that branch was not landed yet.

Prior of landing this branch it would be nice to land the other one and then merge the changes.
https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250

== Tests ==

lp-tt test_pofile_translate

== Demo and Q/A ==

Login as admin.

Unfortunately the default database does not contains any translation messages that can be diverged so you will have to create one.

Go to this translation page and add a translation.
Come back and this time add a suggestion (tick "Someone should review this translation").
No come back one again :)

You should see three check boxes:
"Confirm this translation and dismiss all suggestions." (aka dismiss)
"Make this translation specific to evolution tralala..." (aka diverge)
"Someone should review this translation." (aka force_suggestion)

Click the diverge and dismiss checkboxes and when one is selected the other should be disabled.

Click the dismiss checkbox. Diverge should be still active while all other are disabled.
Checking and un-checking the diverge will not enable force_suggestion.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/canonical/launchpad/javascript/translations/pofile.js

== JSLint notices ==
jslint: Lint found in '/home/adi/launchpad/lp-branches/bug-402235/lib/canonical/launchpad/javascript/translations/pofile.js':
Line 316 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                },

Line 323 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                }, '#' + fields[key], 'down:68+shift+alt', Y, msgset_id);

Line 329 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
                }, '#' + fields[key], 'down:48+shift+alt', Y, fields[key]);

Line 384 character 14: Be careful when making functions within a loop. Consider putting the function in a closure.
            },

Line 403 character 14: Be careful when making functions within a loop. Consider putting the function in a closure.
            },

jslint: 1 file to lint.

To post a comment you must log in.
Revision history for this message
Abel Deuring (adeuring) wrote :
Download full text (4.0 KiB)

Hi Adi,
nice work! I have just a few formal nitpicks, see below.

> === modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
> --- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-11 20:07:25 +0000
> +++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-23 01:14:24 +0000
> @@ -6,9 +6,11 @@
> __metaclass__ = type
> __all__ = []
>
> +import transaction
> +
> from canonical.launchpad.windmill.testing import constants, lpuser
> from lp.translations.windmill.testing import TranslationsWindmillLayer
> -from lp.testing import WindmillTestCase
> +from lp.testing import login, logout, WindmillTestCase
>
>
> class POFileNewTranslationFieldKeybindings(WindmillTestCase):
> @@ -81,3 +83,147 @@
> '_translation_0_new_select')
> self._check_translation_autoselect(
> start_url, new_translation_id, new_translation_select_id)
> +
> +
> +class POFileTranslationActions(WindmillTestCase):
> + """Tests for actions that can be done on a translation message."""
> +
> + layer = TranslationsWindmillLayer
> + suite_name = 'POFile Translation Actions'
> +
> + def test_dismiss_uncheck_force_suggestion(self):
> + """Test the unchecking of force suggestion on dismissal.
> +
> + Checking the dismiss all suggestions checkbox will uncheck a
> + previously checkbox that force submitting the current translation as a

s/previously checkbox/previously ticked checkbox/
s/force/forces/

> + suggestion.
> + """
> +
> + self.test_user = lpuser.TRANSLATIONS_ADMIN
> + # Test the zoom out view for Evolution trunk Spanish (es).
> + url = ('http://translations.launchpad.dev:8085/'
> + 'evolution/trunk/+pots/evolution-2.2/es/5/+translate')
> + dismiss_id = u'msgset_5_dismiss'
> + force_suggestion_id = u'msgset_5_force_suggestion'
> +
> + # Go to the translation page.
> + self.client.open(url=url)
> + self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
> + self.test_user.ensure_login(self.client)
> + self.client.waits.forElement(
> + id=dismiss_id, timeout=constants.FOR_ELEMENT)
> + self.client.waits.forElement(
> + id=force_suggestion_id, timeout=constants.FOR_ELEMENT)
> +
> + # Check that initialy the checkboxes are not selected.

s/initialy/initially/

> + self.client.asserts.assertNotChecked(id=dismiss_id)
> + self.client.asserts.assertNotChecked(id=force_suggestion_id)
> +
> + # Click the force suggestion checkbox and verify that it is checked.
> + self.client.click(id=force_suggestion_id)
> + self.client.asserts.assertChecked(id=force_suggestion_id)
> +
> + self.client.click(id=dismiss_id)
> + self.client.asserts.assertChecked(id=dismiss_id)
> + self.client.asserts.assertNotChecked(id=force_suggestion_id)
> +
> + def test_diverge_and_force_suggestion_mutual_exclusion(self):
> + """Test the mutual exclusion of diverge and force suggestion.
> +
> + Diverge current translation and force suggestion checkbox ...

Read more...

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
2--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-13 12:58:36 +0000
3+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-04-23 16:13:30 +0000
4@@ -18,6 +18,7 @@
5 if (all_dismiss_boxes !== null) {
6 all_dismiss_boxes.each(function(checkbox) {
7 var classbase = checkbox.get('id');
8+ var diverge_id = classbase.replace(/dismiss/, 'diverge');
9 var current_class = classbase.replace(/dismiss/, 'current');
10 var current_radios = Y.all('.' + current_class);
11 var dismissables = Y.all('.' + classbase+'able');
12@@ -30,7 +31,9 @@
13 checkbox.on('click', function(e) {
14 if (checkbox.get('checked')) {
15 dismissables.addClass('dismissed');
16+ Y.all(dismissable_inputs_class).set('checked', false);
17 Y.all(dismissable_inputs_class).set('disabled', true);
18+ Y.all('#' + diverge_id).set('disabled', false);
19 current_radios.set('checked', true);
20 } else {
21 dismissables.removeClass('dismissed');
22@@ -191,6 +194,10 @@
23 (e.shiftKey && e.altKey && e.keyCode == 75)) {
24 return;
25 }
26+
27+ // translation_select_id has one of the following formats:
28+ // * msgset_1_es_translation_0_new_select
29+ // * msgset_2_pt_BR_translation_0_new_select
30 var translation_select_id = field + '_select';
31 selectWidgetByID(translation_select_id);
32 };
33@@ -274,12 +281,10 @@
34 // translation_stem has one of the following formats:
35 // * msgset_1_es
36 // * msgset_2_pt_BR
37- // translation_select_id has one of the following formats:
38- // * msgset_1_es_translation_0_new_select
39- // * msgset_2_pt_BR_translation_0_new_select
40 var html_parts = fields[key].split('_');
41 var msgset_id = html_parts[0] + '_' + html_parts[1];
42- var translation_stem = fields[key].replace(/_translation_(\d)+_new/,"");
43+ var translation_stem = fields[key].replace(
44+ /_translation_(\d)+_new/,"");
45
46 Y.on(
47 'change', selectTranslation,
48@@ -347,8 +352,66 @@
49 };
50
51
52+/**
53+ * Force suggestion and diverge translation checkboxes are mutually excluded.
54+ * Checking one of them will disable the other.
55+ * When the dismiss all suggestion checkbox is checked, uncheking the
56+ * diverte translation checkbox should keep the force suggestion disabled.
57+ */
58+var initializeReviewDivergeMutualExclusion = function (fields) {
59+
60+ // Diverge message field format is 'msgset_ID_diverge'
61+ // Force suggestion field format is 'msgset_ID_force_suggestion'
62+ for (var key = 0; key < fields.length; key++) {
63+ var html_parts = fields[key].split('_');
64+ var msgset_id = html_parts[0] + '_' + html_parts[1];
65+ var diverge_id = msgset_id + '_diverge';
66+ var suggestion_id = msgset_id + '_force_suggestion';
67+ var diverge_checkbox = Y.one('#' + diverge_id);
68+ var suggestion_checkbox = Y.one('#' + suggestion_id);
69+
70+ if (diverge_checkbox === null || suggestion_checkbox === null) {
71+ break;
72+ }
73+
74+ Y.on(
75+ 'click',
76+ function (e) {
77+ if (suggestion_checkbox.get('checked') === true) {
78+ diverge_checkbox.set('disabled', true);
79+ } else {
80+ diverge_checkbox.set('disabled', false);
81+ }
82+ },
83+ suggestion_checkbox);
84+
85+ Y.on(
86+ 'click',
87+ function (e) {
88+ if (diverge_checkbox.get('checked') === true) {
89+ suggestion_checkbox.set('disabled', true);
90+ } else {
91+ // Don't enable the force suggestion checkbox if dismiss
92+ // all suggestions is enabled.
93+ var dismiss_checkbox = Y.one(
94+ '#' + msgset_id + '_dismiss');
95+ if (dismiss_checkbox !== null &&
96+ dismiss_checkbox.get('checked') === true) {
97+ return;
98+ }
99+ suggestion_checkbox.set('disabled', false);
100+ }
101+ },
102+ diverge_checkbox);
103+ }
104+};
105+
106+
107+/**
108+ * Initialize event-key bindings such as moving to the next or previous
109+ * field, or copying original text
110+ */
111 var initializeKeyBindings = function(e) {
112-
113 if (translations_order.length < 1) {
114 // If no translations fiels are displayed on the page
115 // don't initialize the translations order
116@@ -360,10 +423,16 @@
117 fields.push('save_and_continue_button');
118
119 initializeGlobalKeyBindings(fields);
120- initializeFieldsKeyBindings(fields);
121+ initializeFieldsKeyBindings(fields);
122 };
123
124
125+/**
126+ * Translations can be reset by submitting an empty translations and ticking
127+ * the 'Someone should review this translation' checkbox.
128+ * Ticking the 'Someone should review this translation' will automatically
129+ * select the new empty translation.
130+ */
131 var initializeResetBehavior = function (fields) {
132 for (var key = 0; key < fields.length; key++) {
133 var html_parts = fields[key].split('_');
134@@ -405,6 +474,7 @@
135 }
136 };
137
138+
139 /**
140 * Initialize common Javascript code for POFile and TranslationMessage
141 * +translate pages.
142@@ -414,7 +484,6 @@
143 * It will also initializing the reset checkbox behavior and will show the
144 * error notifications.
145 */
146-
147 var initializeBaseTranslate = function () {
148 try {
149 setupSuggestionDismissal();
150@@ -442,11 +511,9 @@
151 }
152 }
153
154-/**
155+
156+/*
157 * Initialize Javascript code for a POFile +translate page.
158- *
159- * This will initialize the base code and will also show the guidelines
160- * if needeed.
161 */
162 self.initializePOFile = function(e) {
163 try {
164@@ -457,15 +524,21 @@
165 initializeBaseTranslate();
166 };
167
168-/**
169+
170+/*
171 * Initialize Javascript code for a TranslationMessage +translate page.
172- *
173- * This will initialize the base code.
174 */
175 self.initializeTranslationMessage = function(e) {
176+
177+ try {
178+ var fields = translations_order.split(' ');
179+ initializeReviewDivergeMutualExclusion(fields);
180+ } catch (e) {
181+ Y.log(e, "error");
182+ }
183+
184 initializeBaseTranslate();
185 };
186
187-
188 }, "0.1", {"requires": ["event", "event-key", "node", "cookie", "anim"]});
189
190
191=== modified file 'lib/lp/translations/templates/currenttranslationmessage-translate-one.pt'
192--- lib/lp/translations/templates/currenttranslationmessage-translate-one.pt 2010-04-07 06:55:23 +0000
193+++ lib/lp/translations/templates/currenttranslationmessage-translate-one.pt 2010-04-23 16:13:30 +0000
194@@ -564,7 +564,8 @@
195 value="diverge_translation"
196 tal:attributes="
197 checked view/force_diverge;
198- name string:${view/html_id}_diverge"
199+ name string:${view/html_id}_diverge;
200+ id string:${view/html_id}_diverge"
201 />
202 <tal:block condition="not:view/is_plural">
203 Make this translation specific to
204
205=== modified file 'lib/lp/translations/windmill/tests/test_pofile_translate.py'
206--- lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-13 12:58:36 +0000
207+++ lib/lp/translations/windmill/tests/test_pofile_translate.py 2010-04-23 16:13:30 +0000
208@@ -6,9 +6,11 @@
209 __metaclass__ = type
210 __all__ = []
211
212+import transaction
213+
214 from canonical.launchpad.windmill.testing import constants, lpuser
215 from lp.translations.windmill.testing import TranslationsWindmillLayer
216-from lp.testing import WindmillTestCase
217+from lp.testing import login, logout, WindmillTestCase
218
219
220 class POFileNewTranslationFieldKeybindings(WindmillTestCase):
221@@ -82,6 +84,150 @@
222 self._checkTranslationAutoselect(
223 start_url, new_translation_id, new_translation_select_id)
224
225+
226+class POFileTranslationActions(WindmillTestCase):
227+ """Tests for actions that can be done on a translation message."""
228+
229+ layer = TranslationsWindmillLayer
230+ suite_name = 'POFile Translation Actions'
231+
232+ def test_dismiss_uncheck_force_suggestion(self):
233+ """Test the unchecking of force suggestion on dismissal.
234+
235+ Checking the dismiss all suggestions checkbox will uncheck a
236+ previously ticked checkbox that forces submitting the current
237+ translation as a suggestion.
238+ """
239+
240+ self.test_user = lpuser.TRANSLATIONS_ADMIN
241+ # Test the zoom out view for Evolution trunk Spanish (es).
242+ url = ('http://translations.launchpad.dev:8085/'
243+ 'evolution/trunk/+pots/evolution-2.2/es/5/+translate')
244+ dismiss_id = u'msgset_5_dismiss'
245+ force_suggestion_id = u'msgset_5_force_suggestion'
246+
247+ # Go to the translation page.
248+ self.client.open(url=url)
249+ self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
250+ self.test_user.ensure_login(self.client)
251+ self.client.waits.forElement(
252+ id=dismiss_id, timeout=constants.FOR_ELEMENT)
253+ self.client.waits.forElement(
254+ id=force_suggestion_id, timeout=constants.FOR_ELEMENT)
255+
256+ # Check that initially the checkboxes are not selected.
257+ self.client.asserts.assertNotChecked(id=dismiss_id)
258+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
259+
260+ # Click the force suggestion checkbox and verify that it is checked.
261+ self.client.click(id=force_suggestion_id)
262+ self.client.asserts.assertChecked(id=force_suggestion_id)
263+
264+ self.client.click(id=dismiss_id)
265+ self.client.asserts.assertChecked(id=dismiss_id)
266+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
267+
268+ def test_diverge_and_force_suggestion_mutual_exclusion(self):
269+ """Test the mutual exclusion of diverge and force suggestion.
270+
271+ Diverge current translation and force suggestion checkbox can not
272+ be enabled at the same time. Checking one option will disable the
273+ other.
274+
275+ If suggestions are dismissed, unchecking the diverge checkbox will
276+ keep the force suggesion disabled.
277+ """
278+
279+ self.test_user = lpuser.TRANSLATIONS_ADMIN
280+ # Test the zoom out view for Evolution trunk Spanish (es).
281+
282+ login('admin@canonical.com')
283+ pofile = self.factory.makePOFile('pt_BR')
284+ potemplate = pofile.potemplate
285+ potmsgset = self.factory.makePOTMsgSet(potemplate)
286+ potmsgset.setSequence(potemplate, 1)
287+ potmsgset_id = potmsgset.id
288+
289+ current_translation = self.factory.makeTranslationMessage(
290+ pofile=pofile, potmsgset=potmsgset, translations=['current'])
291+ transaction.commit()
292+ suggestion = self.factory.makeTranslationMessage(
293+ pofile=pofile, potmsgset=potmsgset,
294+ translations=['suggestion'], suggestion=True)
295+ transaction.commit()
296+ logout()
297+
298+ url = ('http://translations.launchpad.dev:8085/'
299+ '%s/%s/+pots/%s/pt_BR/1/+translate' % (
300+ potemplate.product.name,
301+ potemplate.productseries.name,
302+ potemplate.name))
303+
304+ dismiss_id = u'msgset_%s_dismiss' % potmsgset_id
305+ force_suggestion_id = u'msgset_%s_force_suggestion' % potmsgset_id
306+ diverge_id = u'msgset_%s_diverge' % potmsgset_id
307+
308+ # Go to the translation page.
309+ self.client.open(url=url)
310+ self.client.waits.forPageLoad(timeout=constants.PAGE_LOAD)
311+ self.test_user.ensure_login(self.client)
312+ self.client.waits.forElement(
313+ id=dismiss_id, timeout=constants.FOR_ELEMENT)
314+ self.client.waits.forElement(
315+ id=force_suggestion_id, timeout=constants.FOR_ELEMENT)
316+ self.client.waits.forElement(
317+ id=diverge_id, timeout=constants.FOR_ELEMENT)
318+
319+ # Check that initialy the checkboxes are not selected.
320+ self.client.asserts.assertNotChecked(id=dismiss_id)
321+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
322+ self.client.asserts.assertNotChecked(id=diverge_id)
323+
324+ # Test the diverge translation checking and unchecking.
325+ self.client.click(id=diverge_id)
326+ self.client.asserts.assertChecked(id=diverge_id)
327+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
328+ self.client.asserts.assertNotChecked(id=dismiss_id)
329+ self.client.asserts.assertElemJS(
330+ id=force_suggestion_id, js=u'element.disabled')
331+
332+ self.client.click(id=diverge_id)
333+ self.client.asserts.assertNotChecked(id=diverge_id)
334+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
335+ self.client.asserts.assertNotChecked(id=dismiss_id)
336+ self.client.asserts.assertElemJS(
337+ id=force_suggestion_id, js=u'!element.disabled')
338+
339+ # Test the force suggestion checking and unchecking.
340+ self.client.click(id=force_suggestion_id)
341+ self.client.asserts.assertChecked(id=force_suggestion_id)
342+ self.client.asserts.assertNotChecked(id=diverge_id)
343+ self.client.asserts.assertNotChecked(id=dismiss_id)
344+ self.client.asserts.assertElemJS(
345+ id=diverge_id, js=u'element.disabled')
346+
347+ self.client.click(id=force_suggestion_id)
348+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
349+ self.client.asserts.assertNotChecked(id=diverge_id)
350+ self.client.asserts.assertNotChecked(id=dismiss_id)
351+ self.client.asserts.assertElemJS(
352+ id=diverge_id, js=u'!element.disabled')
353+
354+ # Test unchecking the diverge translations when dismiss all
355+ # suggestions is enabled.
356+ self.client.click(id=dismiss_id)
357+ self.client.asserts.assertElemJS(
358+ id=force_suggestion_id, js=u'element.disabled')
359+ self.client.click(id=diverge_id)
360+ self.client.asserts.assertElemJS(
361+ id=force_suggestion_id, js=u'element.disabled')
362+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
363+
364+ self.client.click(id=diverge_id)
365+ self.client.asserts.assertElemJS(
366+ id=force_suggestion_id, js=u'element.disabled')
367+ self.client.asserts.assertNotChecked(id=force_suggestion_id)
368+
369 def _checkResetTranslationSelect(
370 self, client, checkbox, singular_new_select, singular_current_select,
371 singular_new_field=None, plural_new_select=None):
372@@ -188,7 +334,8 @@
373 checkbox = u'msgset_144_force_suggestion'
374 singular_new_select = u'msgset_144_pt_BR_translation_0_new_select'
375 singular_new_field = u'msgset_144_pt_BR_translation_0_new'
376- singular_current_select = u'msgset_144_pt_BR_translation_0_radiobutton'
377+ singular_current_select = (
378+ u'msgset_144_pt_BR_translation_0_radiobutton')
379 plural_new_select = u'msgset_144_pt_BR_translation_1_new_select'
380 self._checkResetTranslationSelect(
381 client,