Fix #861 -- Add language dropdown #1120

oliverroick · 2017-02-10T09:50:04Z

Proposed changes in this pull request

This PR adds a dropdown to select the language to be used in forms to create and edit locations, relationships, and parties (Fixes #861).

Changes of this PR include:

Add languages.py which defines a list of languages currently supported for multi-language forms. This list can be extended later to support languages that are currently not supported by Django. The list also provides labels that are used in the language dropdown.
Adds core/static/js/xlang.js which takes care of setting the correct translation once the page is loaded and switching the labels when a new language is selected. The selected language is stored in the session store and read from there. If no language is selected or the selected language is not shown the default language is displayed.
Adds set_standard_field to core.form_mixins.AttributeFormMixin to provide translated labels to standard fields (for example location type or party type or party name).
Adds new XLangSelect and XLangSelectMultiple which take care of adding translated labels to options of select and multiple select fields.
Extends create_model_fields of core.form_mixins.AttributeFormMixin so translated labels are provided to all questionnaire form fields.
Extends get_context_data of organization.views.mixin.ProjectMixin to add available languages to the context, which are eventually rendered into the language dropdown.
Extends several forms, so standard fields are translated using `set_standard_field.'
Bump jsonattrs which includes changes to display translated labels in detail views and to generate attributes that indicate translatable labels; used in label and option elements.

When should this PR be merged

Whenever ready

Risks

Low to None

Follow up actions

None

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
Is the PR labeled correctly? It should have the migration label if a new migration is added.
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
If this is a bug fix, are tests for the issue in place There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
If this is a new feature or a significant change to an existing feature has the manual testing spreadsheet been updated with instructions for manual testing?

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

bjohare · 2017-02-13T15:53:06Z

cadasta/core/static/css/forms.scss

+/* =Project translation select dropdown
+-------------------------------------------------------------- */
+
+.langs-select {


Wondering if the language drop down should be styled more like the 'Add location' and 'More actions' dropdowns?

I think the dropdown deliberately looks different; @clash99 can you clarify that?

bjohare

This looks good to me code wise. My only question is the styling of the language drop down, and whether it's in the right place? Maybe it should be moved down to the 'detail' part of the form (beside edit and delete buttons) since it's intended to change the form labels only and not the rest of the UI.. just a thought?

clash99 · 2017-02-14T15:00:46Z

@bjohare - Since the form labels are changed throughout the project - the map, parties, and resource pages - we decided it was best at that top project level. I agree its not ideal and I'll look at it again when we rework those upper right menus.

bjohare · 2017-02-14T15:13:11Z

@clash99 sounds good!

seav

I have several suggestions and questions. Aside from that in some pages, standard fields are not showing the questionnaire-localized labels and/or values:

location type label and value in location detail page
party name label and party type label and value in party details page
party type value in party edit page
relationship type label and value in relationship detail page
(maybe OK?) party name label and party type label and value in party list page
(maybe OK?) relationship type value in location relationship tab

seav · 2017-02-20T08:24:51Z

cadasta/spatial/forms.py

+
+        if self.project.current_questionnaire:
+            self.set_standard_field('party_name',
+                                    _('Please select a party type'),


This empty choice is unnecessary. The field is the party name and you don't select it.

seav · 2017-02-20T08:29:06Z

cadasta/party/tests/test_forms.py


 from .. import forms
 from ..models import Party, TenureRelationshipType


 class PartyFormTest(UserTestCase, TestCase):

+    def test_init_without_form(self):


I think the "form" in the test name (and similar tests) is confusing. It can refer to a Django form or a questionnaire question. Maybe something like test_init_without_labels?

Changed it to "questionnaire"

seav · 2017-02-20T09:00:52Z

cadasta/core/static/js/xlang.js

+
+  $('#form-langs-select').change(function() {
+    const new_lang = $(this).val();
+    sessionStorage.setItem("form_lang", new_lang);


Why sessionStorage instead of localStorage? Using sessionStorage means that the selection won't persist in different browser sessions.

seav · 2017-02-20T09:21:19Z

cadasta/organization/views/mixins.py

@@ -113,7 +114,25 @@ def is_administrator(self):

    def get_context_data(self, *args, **kwargs):


Why not update this method to the "usual" style where we get the context data from super() and then inserting additional context data?

seav · 2017-02-20T09:22:31Z

cadasta/organization/views/mixins.py

        return super().get_context_data(is_project_member=prj_member,
+                                        form_lang_default=form_lang_default,
+                                        form_langs=sorted(form_langs,


You're sorting form_langs when it is already sorted earlier?

That's a very good question. I removed the second sorted

seav · 2017-02-20T09:33:54Z

cadasta/core/form_mixins.py

+                choices = QuestionOption.objects.filter(
+                    question=question).values_list('name', 'label_xlat')
+                choices, xlang_labels = zip(*[((c[0], c[1].get(default_lang)),
+                                              (c[0], c[1])) for c in choices])


If I use a questionnaire that has only 1 set of labels (only the default language), I get the following error on this line when viewing HTML forms with select fields:

'str' object has no attribute 'get'

I think we need test cases for these types of questionnaires.

That's another one of those examples, where inconsistent use of XLSForms creates problems. If you name the the labels column label::en and define the default language this works fine. Only if you don't specify the language it breaks. But I suppose we already have questionnaires without a specified language in our database, so I'm going to have to spend another hour of my life now to work around this.

seav · 2017-02-20T09:39:17Z

cadasta/spatial/forms.py

@@ -40,6 +40,11 @@ def __init__(self, project=None, *args, **kwargs):
        self.project = project
        self.add_attribute_fields()

+        if self.project.current_questionnaire:
+            self.set_standard_field('location_type',
+                                    _('Please select a location type'),


I think we should remove the empty choice when the form is used for editing, similar to how we do it for tenure types.

If there is no questionnaire for the project, this select field will also have an empty option when the form is used for editing, so I'm not going to change it.

seav · 2017-02-20T09:43:35Z

cadasta/templates/organization/project_wrapper.html

+      <!-- Project translation dropdown-->
+      {% if form_langs %}
+      <div class="langs-select form-group pull-right">
+        <label class="control-label sr-only" for="form-langs-select">Language for project fields</label>


Wrap the label text in {% trans %}.

seav · 2017-02-20T09:50:53Z

cadasta/templates/spatial/location_form.html

@@ -23,14 +23,15 @@
          </div>
          <h3>{% trans "Overview" %}</h3>
          <div class="form-group{% if form.type.errors %} has-error{% endif %}">
-            <label class="control-label" for="{{ form.type.id_for_label }}">{% trans "2. What type of location is this?" %}</label>
+            <label class="control-label" for="{{ form.type.id_for_label }}" {{ form.type.field.labels_xlang|safe }}>{% trans "2. What type of location is this?" %}</label>


Since we are translating the label if there are questionnaire labels here, I guess we should remove the "2." in the default label and the earlier "1." in the location geometry section. When labels are translated, the "1." looks weird by itself.

oliverroick · 2017-02-21T11:43:27Z

@seav Addressed your change requests. Needs another review because the latest commit adds quite a lot.

seav · 2017-02-21T13:30:36Z

@oliverroick, I'm currently reviewing it now. So far everything looks good and the single-translation questionnaire is no longer throwing up errors. I did notice one minor corner-case defect. When the localStorage (or sessionStorage) doesn't yet have the form_lang setting, the standard fields are shown in the platform language instead of the questionnaire's default language while the other fields are shown in the default language. This behavior persists until the language drop-down is first changed because that's the only time localStorage gets the setting. Maybe if the setting has not yet been stored, we should auto-select and store the default language?

seav · 2017-02-21T14:13:15Z

Also another scenario entered my mind. We are storing the selected language under the key form_lang in localStorage. Do we need to prefix this with the project slug or something so that the user can set the selected language per project? If a user accesses multiple projects, all of which may or may not have matching questionnaire languages, they would all use the same localStorage setting. This means that the selected language would be reset every time a different project is viewed.

oliverroick · 2017-02-22T10:55:02Z

When the localStorage (or sessionStorage) doesn't yet have the form_lang setting, the standard fields are shown in the platform language instead of the questionnaire's default language while the other fields are shown in the default language. This behavior persists until the language drop-down is first changed because that's the only time localStorage gets the setting. Maybe if the setting has not yet been stored, we should auto-select and store the default language?

I addressed this in the latest commit. When the page is loaded and no language has been selected before, the JS selects the default language and sets it as the selected language in localStorage

Also another scenario entered my mind. We are storing the selected language under the key form_lang in localStorage. Do we need to prefix this with the project slug or something so that the user can set the selected language per project? If a user accesses multiple projects, all of which may or may not have matching questionnaire languages, they would all use the same localStorage setting. This means that the selected language would be reset every time a different project is viewed.

I addressed this as well, because given the changes above it would make behaviour weird when switching between projects. The selected language is stored for each project in localStorage using the key form_lang::{project_slug}.

oliverroick added PR: needs review PR: needs testing labels Feb 10, 2017

oliverroick requested review from seav and bjohare February 10, 2017 09:50

bjohare reviewed Feb 13, 2017

View reviewed changes

bjohare approved these changes Feb 14, 2017

View reviewed changes

oliverroick mentioned this pull request Feb 14, 2017

Including relationship details in party detail page #1001

Closed

oliverroick force-pushed the feature/language-dropdown branch 2 times, most recently from f3205a4 to c47ada7 Compare February 16, 2017 15:19

seav suggested changes Feb 20, 2017

View reviewed changes

oliverroick force-pushed the feature/language-dropdown branch from 316d196 to 785cd9a Compare February 22, 2017 10:50

seav approved these changes Feb 22, 2017

View reviewed changes

seav added PR: approved and removed PR: needs review labels Feb 22, 2017

bohare and others added 4 commits February 28, 2017 16:18

Fix #861 -- Add language dropdown

f1e4776

Make sure langauge in session store can be selected

98aba30

Address change requests

f62e85c

Select default lang for a form; store selected lang per project

6ba66de

oliverroick force-pushed the feature/language-dropdown branch from 785cd9a to 6ba66de Compare February 28, 2017 15:19

oliverroick merged commit 3fb2ce7 into master Feb 28, 2017

oliverroick deleted the feature/language-dropdown branch February 28, 2017 15:58

oliverroick mentioned this pull request Mar 1, 2017

Fix #1009 -- Customize list of accepted questionnaire languages #1168

Merged

14 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #861 -- Add language dropdown #1120

Fix #861 -- Add language dropdown #1120

oliverroick commented Feb 10, 2017

bjohare Feb 13, 2017

oliverroick Feb 14, 2017

bjohare left a comment

clash99 commented Feb 14, 2017

bjohare commented Feb 14, 2017

seav left a comment

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

seav Feb 20, 2017

oliverroick Feb 20, 2017

oliverroick commented Feb 21, 2017

seav commented Feb 21, 2017 •

edited

Loading

seav commented Feb 21, 2017 •

edited

Loading

oliverroick commented Feb 22, 2017 •

edited

Loading

		@@ -113,7 +114,25 @@ def is_administrator(self):

		def get_context_data(self, args, *kwargs):

Fix #861 -- Add language dropdown #1120

Fix #861 -- Add language dropdown #1120

Conversation

oliverroick commented Feb 10, 2017

Proposed changes in this pull request

When should this PR be merged

Risks

Follow up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjohare left a comment

Choose a reason for hiding this comment

clash99 commented Feb 14, 2017

bjohare commented Feb 14, 2017

seav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverroick commented Feb 21, 2017

seav commented Feb 21, 2017 • edited Loading

seav commented Feb 21, 2017 • edited Loading

oliverroick commented Feb 22, 2017 • edited Loading

seav commented Feb 21, 2017 •

edited

Loading

seav commented Feb 21, 2017 •

edited

Loading

oliverroick commented Feb 22, 2017 •

edited

Loading