launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #00219
[Merge] lp:~jtv/launchpad/bug-600673 into lp:launchpad/devel
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-600673 into lp:launchpad/devel.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers): js
= Bug 600673 =
This is a time-saving feature for those of us who have to review translation import queue entries. There are not many who do this, but plenty of queue to get through so this matters.
It's all JavaScript. When reviewing an entry for a translation upload, you see a template selector dropdown. This lets you select which template (in the given translation target) this file is a translation of. The change in this branch is that the same dropdown will show up for template uploads.
The dropdown's HTML value is meaningless for template uploads, which is why it was previously hidden when reviewing these entries. Instead, if you decide that the template is an update of an existing one, you need to look up the exact name and domain for that template. This is highly tedious work: navigate to the target's templates list, click through to the template, proceed to its administration page, look up the exact name and template, CLOSE THE ADMINISTRATION PAGE before you change anything by accident, and reproduce the name and domain in the approval form. If you got distracted somewhere in this procedure and are no longer sure about the exact spelling, repeat.
The dropdown's new role in this scenario is to fill out the name and domain fields automatically. Just select an existing template from the dropdown and you're done. Inelegantly different from its role in translation uploads, but highly effective.
As demonstrated by the windmill test (which took blood, sweat, tears and help from Salgado to get running) the dropdown has some user-friendly but not immediately obvious behaviours:
* If the default name/domain for the upload match those of an existing template, it initializes itself to that template.
* If you start out with an existing template selected but then hand-edit the name and/or domain, it goes back to "none."
* If you type right name/domain for an existing template into the input fields, it again selects that template.
* If you customize the name/domain, then select an existing template, then select "none" again, you get your last input values back.
To test this, run:
{{{
./bin/test -vvc -m lp.translations.windmill -t import_queue
}}}
To Q/A, play with an import form. I've been doing that for ages (see demo attached to the bug) and am happy with it, but the windmill tests took longer. In fact I even fixed a bug in the login sequence for an existing test in the same test case. And fun facts of the day:
* The value codes in the HTML dropdown appear more or less arbitrary, so use the "option" parameter to select an option by its user-visible representation.
* assertText doesn't work for an input field, but it doesn't necessarily fail either. It just finds an empty string, resulting in an obscure test failure.
Jeroen
--
https://code.launchpad.net/~jtv/launchpad/bug-600673/+merge/30679
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-600673 into lp:launchpad/devel.
=== modified file 'BRANCH.TODO'
--- BRANCH.TODO 2010-06-25 15:03:10 +0000
+++ BRANCH.TODO 2010-07-22 16:13:55 +0000
@@ -2,3 +2,4 @@
# landing. There is a test to ensure it is empty in trunk. If there is
# stuff still here when you are ready to land, the items should probably
# be converted to bugs so they can be scheduled.
+ * Escape template names/domains in js_domain_mapping.
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-07-21 14:41:26 +0000
+++ lib/lp/testing/factory.py 2010-07-22 16:13:55 +0000
@@ -162,6 +162,10 @@
RosettaImportStatus)
from lp.translations.interfaces.translationgroup import (
ITranslationGroupSet)
+from lp.translations.interfaces.translationimportqueue import (
+ RosettaImportStatus)
+from lp.translations.interfaces.translationfileformat import (
+ TranslationFileFormat)
from lp.translations.interfaces.translationsperson import ITranslationsPerson
from lp.translations.interfaces.translationtemplatesbuildjob import (
ITranslationTemplatesBuildJobSource)
@@ -2061,18 +2065,25 @@
translation.is_imported = is_imported
translation.is_current = True
- def makeTranslationImportQueueEntry(self, path=None, status=None,
+ def makeTranslationImportQueueEntry(self, path=None, productseries=None,
+ distroseries=None,
sourcepackagename=None,
- distroseries=None,
- productseries=None, content=None,
- uploader=None, is_published=False):
+ potemplate=None, content=None,
+ uploader=None, pofile=None,
+ format=None, status=None):
"""Create a `TranslationImportQueueEntry`."""
if path is None:
path = self.getUniqueString() + '.pot'
- if status is None:
- status = RosettaImportStatus.NEEDS_REVIEW
-
- if (sourcepackagename is None and distroseries is None):
+
+ for_distro = not (distroseries is None and sourcepackagename is None)
+ for_project = productseries is not None
+ if not for_distro and not for_project and potemplate is not None:
+ # Copy target from template.
+ distroseries = potemplate.distroseries
+ sourcepackagename = potemplate.sourcepackagename
+ productseries = potemplate.productseries
+
+ if sourcepackagename is None and distroseries is None:
if productseries is None:
productseries = self.makeProductSeries()
else:
@@ -2086,16 +2097,21 @@
if content is None:
content = self.getUniqueString()
-
content_reference = getUtility(ILibraryFileAliasSet).create(
name=os.path.basename(path), size=len(content),
file=StringIO(content), contentType='text/plain')
+ if format is None:
+ format = TranslationFileFormat.PO
+
+ if status is None:
+ status = RosettaImportStatus.NEEDS_REVIEW
+
return TranslationImportQueueEntry(
- path=path, status=status, sourcepackagename=sourcepackagename,
- distroseries=distroseries, productseries=productseries,
- importer=uploader, content=content_reference,
- is_published=is_published)
+ path=path, productseries=productseries, distroseries=distroseries,
+ sourcepackagename=sourcepackagename, importer=uploader,
+ content=content_reference, status=status, format=format,
+ is_published=False)
def makeMailingList(self, team, owner):
"""Create a mailing list for the team."""
=== modified file 'lib/lp/translations/browser/translationimportqueue.py'
--- lib/lp/translations/browser/translationimportqueue.py 2010-04-19 15:24:29 +0000
+++ lib/lp/translations/browser/translationimportqueue.py 2010-07-22 16:13:55 +0000
@@ -232,8 +232,8 @@
def initialize(self):
"""Remove some fields based on the entry handled."""
self.field_names = ['file_type', 'path', 'sourcepackagename',
+ 'potemplate', 'potemplate_name',
'name', 'translation_domain', 'languagepack',
- 'potemplate', 'potemplate_name',
'language', 'variant']
if self.context.productseries is not None:
@@ -508,6 +508,20 @@
self.context.setStatus(RosettaImportStatus.APPROVED, self.user)
self.context.date_status_changed = UTC_NOW
+ @property
+ def js_domain_mapping(self):
+ """Return JS code mapping templates' names to translation domains."""
+ target = self.import_target
+ if target is None:
+ contents = ""
+ else:
+# XXX: Even with weird characters disallowed, we should escape these.
+ contents = ", \n".join([
+ "'%s': '%s'" % (template.name, template.translation_domain)
+ for template in target.getCurrentTranslationTemplates()
+ ])
+ return "var template_domains = {%s};" % contents
+
class TranslationImportQueueNavigation(GetitemNavigation):
usedfor = ITranslationImportQueue
=== modified file 'lib/lp/translations/javascript/importqueue.js'
--- lib/lp/translations/javascript/importqueue.js 2010-07-15 10:55:27 +0000
+++ lib/lp/translations/javascript/importqueue.js 2010-07-22 16:13:55 +0000
@@ -169,18 +169,11 @@
* Set up the import queue page.
*/
namespace.initialize_import_queue_page = function (Y) {
- // Show spinner while patching up the page, pre-fetchig the image.
- var spinner_loader = Y.all('#spinner-loader');
- spinner_loader.set('innerHTML', '<img src="/@@/spinner" alt="" />');
-
// Set up error output buttons.
init_error_output_buttons();
// Set up status pickers.
Y.all('.status-choice').each(init_status_choice);
-
- // Remove the spinner again.
- spinner_loader.set('innerHTML', '');
};
}, "0.1", {
=== modified file 'lib/lp/translations/javascript/importqueueentry.js'
--- lib/lp/translations/javascript/importqueueentry.js 2010-07-01 10:44:14 +0000
+++ lib/lp/translations/javascript/importqueueentry.js 2010-07-22 16:13:55 +0000
@@ -15,7 +15,7 @@
* @private
*/
var field_groups = {
- 'POT': ['name', 'translation_domain', 'languagepack'],
+ 'POT': ['potemplate', 'name', 'translation_domain', 'languagepack'],
'PO': ['potemplate', 'potemplate_name', 'language', 'variant'],
'UNSPEC': []
};
@@ -25,6 +25,11 @@
var hidden_field_class = 'unseen';
+/* Last selected template name and translation domain that are not among
+ * the options in the templates dropdown.
+ */
+var custom_template_name, custom_translation_domain;
+
/**
* Find a page element by HTML id.
@@ -57,6 +62,18 @@
/**
+ * Retrieve current value of a form field, or null if it does not exist.
+ * @private
+ * @method getFormFieldValue
+ * @param field_name Zope form field name.
+ */
+function getFormFieldValue(field_name) {
+ var field = getFormField(field_name);
+ return (field === null) ? null : field.get('value');
+}
+
+
+/**
* Find the table row that contains the given form field.
* @private
* @method getFormEntry
@@ -75,6 +92,32 @@
/**
+ * Is the form field of the given name currently visible?
+ * @private
+ * @method isFieldVisible
+ * @param field_name Zope form field name.
+ */
+function isFieldVisible(field_name) {
+ var entry = getFormEntry(field_name);
+ return (entry === null) ? false : !entry.hasClass(hidden_field_class);
+}
+
+
+/**
+ * If field is shown, update its value.
+ * @private
+ * @method updateFieldInputIfShown
+ * @param field_name Zope form field name to update.
+ * @param value New value for input field.
+ */
+function updateFieldInputIfShown(field_name, value) {
+ if (isFieldVisible(field_name)) {
+ getFormField(field_name).set('value', value);
+ }
+}
+
+
+/**
* Apply function `alteration` to the form fields for a given file type.
* @private
* @method alterFields
@@ -116,9 +159,7 @@
var showElement = function (element) {
element.removeClass(hidden_field_class);
};
- alterFields(file_type, function (element) {
- element.removeClass(hidden_field_class);
- });
+ alterFields(file_type, showElement);
if (interactively) {
// Animate revealed fields.
@@ -133,6 +174,71 @@
/**
+ * Update the state of the templates dropdown to reflect the current
+ * contents of the name and translation_domain fields.
+ */
+function updateTemplatesDropdown() {
+ var name = getFormFieldValue('name');
+ var domain = getFormFieldValue('translation_domain');
+ var potemplate_dropdown = getFormField('potemplate');
+ var options = potemplate_dropdown.get('options');
+ var num_options = options.size();
+
+ for (var option_index = 0; option_index < num_options; option_index++) {
+ var option = options.item(option_index);
+ var known_name = option.get('textContent');
+ if (known_name == name && template_domains[known_name] == domain) {
+ // The current template name/domain are in the dropdown's
+ // options. Select that option.
+ potemplate_dropdown.set('selectedIndex', option_index);
+ return;
+ }
+ }
+
+ // No match. Select no template.
+ potemplate_dropdown.set('selectedIndex', 0);
+ custom_template_name = name;
+ custom_translation_domain = domain;
+}
+
+
+/**
+ * Handle a change to the templates dropdown.
+ */
+function handleTemplateChoice(e) {
+ var dropdown = e.target;
+ var option_index = dropdown.get('selectedIndex');
+ var name, domain;
+ if (option_index > 0) {
+ // Template selected. Use its name and translation domain.
+ name = dropdown.get('options').item(option_index).get('text');
+ domain = template_domains[name]
+ } else {
+ // No template selected. Pick whatever we had for this case.
+ name = custom_template_name;
+ domain = custom_translation_domain;
+ }
+ updateFieldInputIfShown('name', name);
+ updateFieldInputIfShown('translation_domain', domain);
+}
+
+
+/**
+ * Set up the templates dropdown and related event handlers.
+ */
+function setUpTemplatesChoice() {
+ custom_template_name = getFormFieldValue('name');
+ custom_translation_domain = getFormFieldValue('translation_domain');
+ updateTemplatesDropdown();
+
+ getFormField('potemplate').on('change', handleTemplateChoice)
+
+ getFormField('name').on('change', updateTemplatesDropdown);
+ getFormField('translation_domain').on('change', updateTemplatesDropdown);
+}
+
+
+/**
* Handle change event for current file type.
* @private
* @method handleFileTypeChange
@@ -153,6 +259,7 @@
var file_type_field = getFormField('file_type');
var preselected_file_type = file_type_field.get('value');
updateCurrentFileType(preselected_file_type, false);
+ setUpTemplatesChoice();
file_type_field.on('change', handleFileTypeChange);
};
=== modified file 'lib/lp/translations/templates/translation-import-queue-macros.pt'
--- lib/lp/translations/templates/translation-import-queue-macros.pt 2010-05-18 18:04:00 +0000
+++ lib/lp/translations/templates/translation-import-queue-macros.pt 2010-07-22 16:13:55 +0000
@@ -186,7 +186,6 @@
</form>
</tal:block>
</div>
- <div id="spinner-loader"></div>
</metal:translation-import-queue>
</body>
</html>
=== modified file 'lib/lp/translations/templates/translationimportqueueentry-index.pt'
--- lib/lp/translations/templates/translationimportqueueentry-index.pt 2010-06-29 15:42:35 +0000
+++ lib/lp/translations/templates/translationimportqueueentry-index.pt 2010-07-22 16:13:55 +0000
@@ -13,6 +13,11 @@
display: none;
}
</style>
+
+ <script type="text/javascript"
+ tal:content="view/js_domain_mapping">
+ var template_domains = {'name1': 'domain1', 'name2': 'domain2'};
+ </script>
<script type="text/javascript">
LPS.use('node', 'lp.translations.importqueueentry',
function (Y) {
=== modified file 'lib/lp/translations/windmill/tests/test_import_queue.py'
--- lib/lp/translations/windmill/tests/test_import_queue.py 2010-06-29 19:11:31 +0000
+++ lib/lp/translations/windmill/tests/test_import_queue.py 2010-07-22 16:13:55 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Test for translation import queue entry approving behaviour."""
@@ -11,6 +11,7 @@
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from canonical.launchpad.webapp import canonical_url
from canonical.launchpad.windmill.testing.constants import (
FOR_ELEMENT, PAGE_LOAD, SLEEP)
from canonical.launchpad.windmill.testing import lpuser
@@ -29,6 +30,7 @@
FIELDS = {
'POT': [
+ 'field.potemplate',
'field.name',
'field.translation_domain',
],
@@ -54,22 +56,21 @@
u"//%s[@id='%s']" % (input_tag, field_id)
)
- def _assertAllFieldsVisible(self, client, groupname):
- """Assert that all fields in this group are visible.
+ def _assertAllFieldsVisible(self, client, fields):
+ """Assert that all given fields are visible.
- Fields are visible if they do not have the dont_show_fields
- class set.
+ Fields are visible if they do not have the "unseen" class set.
"""
- for field_id in self.FIELDS[groupname]:
+ for field_id in fields:
client.asserts.assertNotNode(
xpath=self._getHiddenTRXpath(field_id))
- def _assertAllFieldsHidden(self, client, groupname):
- """Assert that all fields in this group are hidden.
+ def _assertAllFieldsHidden(self, client, fields):
+ """Assert that all given fields are hidden.
- Fields are hidden if they have the dont_show_fields class set.
+ Fields are hidden if they have the "unseen" class set.
"""
- for field_id in self.FIELDS[groupname]:
+ for field_id in fields:
client.asserts.assertNode(
xpath=self._getHiddenTRXpath(field_id))
@@ -79,27 +80,134 @@
start_url = 'http://translations.launchpad.dev:8085/+imports/1'
user = lpuser.TRANSLATIONS_ADMIN
# Go to import queue page logged in as translations admin.
+ user.ensure_login(client)
client.open(url=start_url)
client.waits.forPageLoad(timeout=PAGE_LOAD)
- user.ensure_login(client)
+
+ po_only_fields = set(self.FIELDS['PO']) - set(self.FIELDS['POT'])
+ pot_only_fields = set(self.FIELDS['POT']) - set(self.FIELDS['PO'])
# When the page is first called the file_type is set to POT and
# only the relevant form fields are displayed. When the file type
# select box is changed to PO, other fields are shown hidden while
# the first ones are hidden. Finally, all fields are hidden if the
# file type is unspecified.
- client.waits.forElement(id=u'field.file_type', timeout=u'8000')
+ client.waits.forElement(id=u'field.file_type', timeout=FOR_ELEMENT)
client.asserts.assertSelected(id=u'field.file_type', validator=u'POT')
- self._assertAllFieldsVisible(client, 'POT')
- self._assertAllFieldsHidden(client, 'PO')
+ self._assertAllFieldsVisible(client, self.FIELDS['POT'])
+ self._assertAllFieldsHidden(client, po_only_fields)
client.select(id=u'field.file_type', val=u'PO')
- self._assertAllFieldsVisible(client, 'PO')
- self._assertAllFieldsHidden(client, 'POT')
+ self._assertAllFieldsVisible(client, self.FIELDS['PO'])
+ self._assertAllFieldsHidden(client, pot_only_fields)
client.select(id=u'field.file_type', val=u'UNSPEC')
- self._assertAllFieldsHidden(client, 'POT')
- self._assertAllFieldsHidden(client, 'PO')
+ self._assertAllFieldsHidden(client, self.FIELDS['POT'])
+ self._assertAllFieldsHidden(client, self.FIELDS['PO'])
+
+ def test_import_queue_entry_template_selection_new_path(self):
+ # For template uploads, the template dropdown controls the
+ # template name & translation domain fields. This saves the
+ # trouble of finding the exact strings when approving an update
+ # with path changes.
+ # If the upload's path does not match any existing templates,
+ # the dropdown has no template pre-selected.
+
+ # A productseries has two templates.
+ template1 = self.factory.makePOTemplate(
+ path='1.pot', name='name1', translation_domain='domain1')
+ productseries = template1.productseries
+ template2 = self.factory.makePOTemplate(
+ path='2.pot', name='name2', translation_domain='domain2')
+
+ # A new template upload for that entry has a path that doesn't
+ # match either of the existing templates.
+ base_filename = unicode(self.factory.getUniqueString(prefix='x'))
+ path = base_filename + '.pot'
+ entry = self.factory.makeTranslationImportQueueEntry(
+ path, productseries=productseries)
+ url = canonical_url(entry, rootsite='translations')
+
+ transaction.commit()
+
+ # The client reviews the upload.
+ client = self.client
+ user = lpuser.TRANSLATIONS_ADMIN
+ user.ensure_login(client)
+ client.open(url=url)
+ client.waits.forPageLoad(timeout=PAGE_LOAD)
+ client.waits.sleep(milliseconds=SLEEP)
+
+ # No template is preselected in the templates dropdown.
+ client.waits.forElement(id=u'field.potemplate', timeout=FOR_ELEMENT)
+ client.asserts.assertSelected(id=u'field.potemplate', validator=u'')
+
+ # The template name and translation domain are pre-set to a
+ # guess based on the base filename.
+ client.waits.forElement(id=u'field.name', timeout=FOR_ELEMENT)
+ client.asserts.assertText(
+ id=u'field.name', validator=base_filename)
+ client.waits.forElement(
+ id=u'field.translation_domain', timeout=FOR_ELEMENT)
+ client.asserts.assertText(
+ id=u'field.translation_domain', validator=base_filename)
+
+ # The user edits the name and translation domain.
+ client.type(id=u'field.name', text='new-name')
+ client.type(id=u'field.translation_domain', text='new-domain')
+
+ # The user selects an existing template from the dropdown. This
+ # updates the name and domain fields.
+ client.select(id=u'field.potemplate', val=u'name1')
+ client.asserts.assertText(id=u'field.name', validator='name1')
+ client.asserts.assertText(
+ id=u'field.translation_domain', validator='domain1')
+
+ # When the user returns the dropbox to its original state, the
+ # last-entered name and domain come back.
+ client.select(id=u'field.potemplate', val=u'')
+ client.asserts.assertText(id=u'field.name', validator='new-name')
+ client.asserts.assertText(
+ id=u'field.translation_domain', validator='new-domain')
+
+ # Changing the name and domain to those of an existing domain
+ # also updates the dropdown.
+ client.type(id=u'field.name', text=u'name1')
+ client.type(id=u'field.translation_domain', text=u'domain1')
+ client.asserts.assertSelected(id=u'field.potemplate', val=u'name1')
+
+ def test_import_queue_entry_template_selection_existing_path(self):
+ # If a template upload's base path matches the name/domain of an
+ # existing template, the templates dropdown has that template
+ # pre-selected.
+ template_name = self.factory.getUniqueString()
+ template_path = template_name + '.pot'
+ template = self.factory.makePOTemplate(
+ path=template_path, name=template_name,
+ translation_domain=template_name)
+
+ # A new template upload for that entry has a path that doesn't
+ # match either of the existing templates.
+ entry = self.factory.makeTranslationImportQueueEntry(
+ path=template_path, productseries=template.productseries,
+ distroseries=template.distroseries,
+ sourcepackagename=template.sourcepackagename)
+ file('/tmp/foo.log','a').write("* %d: %s *\n"%(entry.id,entry.path))
+ url = canonical_url(entry, rootsite='translations')
+
+ transaction.commit()
+
+ # The client reviews the upload.
+ client = self.client
+ user = lpuser.TRANSLATIONS_ADMIN
+ user.ensure_login(client)
+ client.open(url=url)
+ client.waits.forPageLoad(timeout=PAGE_LOAD)
+ client.waits.sleep(milliseconds=SLEEP)
+
+ # The matching template is preselected in the templates dropdown.
+ client.asserts.assertSelected(
+ id=u'field.potemplate', validator=template_name)
IMPORT_STATUS = u"//tr[@id='%d']//span[contains(@class,'status-choice')]"
Follow ups
