From 72230448a7ebdd47087e38df5c5b20dda3e685b4 Mon Sep 17 00:00:00 2001 From: Andrew Dickinson Date: Mon, 12 Jul 2021 16:48:19 -0400 Subject: [PATCH] Improve localization on the History page using string replacement (#587) --- Makefile | 2 +- ihatemoney/history.py | 10 ++- ihatemoney/run.py | 5 ++ ihatemoney/templates/history.html | 102 +++++++++++++++++++----------- ihatemoney/tests/history_test.py | 4 +- ihatemoney/utils.py | 70 +++++++++++++++++++- 6 files changed, 147 insertions(+), 46 deletions(-) diff --git a/Makefile b/Makefile index 2e8c18ff..ec19e163 100644 --- a/Makefile +++ b/Makefile @@ -71,7 +71,7 @@ build-translations: ## Build the translations .PHONY: update-translations update-translations: ## Extract new translations from source code - $(VENV)/bin/pybabel extract --strip-comments --omit-header --no-location --mapping-file ihatemoney/babel.cfg -o ihatemoney/messages.pot ihatemoney + $(VENV)/bin/pybabel extract --add-comments "I18N:" --strip-comments --omit-header --no-location --mapping-file ihatemoney/babel.cfg -o ihatemoney/messages.pot ihatemoney $(VENV)/bin/pybabel update -i ihatemoney/messages.pot -d ihatemoney/translations/ .PHONY: create-database-revision diff --git a/ihatemoney/history.py b/ihatemoney/history.py index 3f99420a..4c2ab081 100644 --- a/ihatemoney/history.py +++ b/ihatemoney/history.py @@ -1,4 +1,3 @@ -from flask_babel import gettext as _ from sqlalchemy_continuum import Operation, parent_class from ihatemoney.models import BillVersion, Person, PersonVersion, ProjectVersion @@ -69,11 +68,10 @@ def get_history(project, human_readable_names=True): history = [] for version_list in [person_query.all(), project_query.all(), bill_query.all()]: for version in version_list: - object_type = { - "Person": _("Participant"), - "Bill": _("Bill"), - "Project": _("Project"), - }[parent_class(type(version)).__name__] + object_type = parent_class(type(version)).__name__ + + # The history.html template can only handle objects of these types + assert object_type in ["Person", "Bill", "Project"] # Use the old name if applicable if version.previous: diff --git a/ihatemoney/run.py b/ihatemoney/run.py index fc927e44..59fd537a 100644 --- a/ihatemoney/run.py +++ b/ihatemoney/run.py @@ -8,6 +8,7 @@ from flask_babel import Babel, format_currency from flask_mail import Mail from flask_migrate import Migrate, stamp, upgrade from jinja2 import pass_context +from markupsafe import Markup from werkzeug.middleware.proxy_fix import ProxyFix from ihatemoney import default_settings @@ -17,7 +18,9 @@ from ihatemoney.models import db from ihatemoney.utils import ( IhmJSONEncoder, PrefixedWSGI, + em_surround, locale_from_iso, + localize_list, minimal_round, static_include, ) @@ -150,6 +153,8 @@ def create_app( app.jinja_env.globals["static_include"] = static_include app.jinja_env.globals["locale_from_iso"] = locale_from_iso app.jinja_env.filters["minimal_round"] = minimal_round + app.jinja_env.filters["em_surround"] = lambda text: Markup(em_surround(text)) + app.jinja_env.filters["localize_list"] = localize_list # Translations and time zone (used to display dates). The timezone is # taken from the BABEL_DEFAULT_TIMEZONE settings, and falls back to diff --git a/ihatemoney/templates/history.html b/ihatemoney/templates/history.html index d4965d86..bec558ff 100644 --- a/ihatemoney/templates/history.html +++ b/ihatemoney/templates/history.html @@ -29,13 +29,14 @@ {% endif %} {% endmacro %} -{% macro describe_object(event) %}{{ event.object_type }} {{ event.object_desc }}{% endmacro %} - -{% macro simple_property_change(event, localized_property_name, from=True) %} - {{ describe_object(event) }}: - {{ localized_property_name }} {{ _("changed") }} - {% if from %}{{ _("from") }} {{ event.val_before }}{% endif %} - {{ _("to") }} {{ event.val_after }} +{% macro bill_property_change(event, localized_property_name, before=event.val_before|em_surround, after=event.val_after|em_surround) %} + {% set name=event.object_desc|em_surround %} + {% set property_name=localized_property_name %} + {% if before %} + {% trans %}Bill {{ name }}: {{ property_name }} changed from {{ before }} to {{ after }}{% endtrans %} + {% else %} + {% trans %}Bill {{ name }}: {{ property_name }} changed to {{ after }}{% endtrans %} + {% endif %} {% endmacro %} {% macro clear_history_modals() %} @@ -83,17 +84,13 @@ {% endmacro %} {% macro owers_changed(event, add) %} - {{ describe_object(event) }}: {% if add %}{{ _("Added") }}{% else %}{{ _("Removed") }}{% endif %} - {% if event.val_after|length > 1 %} - {% for name in event.val_after %} - {{ name }}{% if event.val_after|length > 2 and loop.index != event.val_after|length %},{% endif %} - {% if loop.index == event.val_after|length - 1 %} {{ _("and") }} {% endif %} - {% endfor %} + {% set name=event.object_desc|em_surround %} + {% set owers_list_str=event.val_after|localize_list|safe %} + {% if add %} + {% trans %}Bill {{ name }}: added {{ owers_list_str }} to owers list{% endtrans %} {% else %} - {{ event.val_after[0] }} + {% trans %}Bill {{ name }}: removed {{ owers_list_str }} from owers list{% endtrans %} {% endif %} - {% if add %}{{ _("to") }}{% else %}{{ _("from") }}{% endif %} - {{ _("owers list") }} {% endmacro %} {% block sidebar %} @@ -174,53 +171,86 @@ >
+ {# Common value setting #} + {% set name=event.object_desc|em_surround %} + {% if event.operation_type == OperationType.INSERT %} - {{ event.object_type }} {{ event.object_desc }} {{ _("added") }} + {% if event.object_type == "Project" %} + {% trans %}Project {{ name }} added{% endtrans %} + {% elif event.object_type == "Bill" %} + {% trans %}Bill {{ name }} added{% endtrans %} + {% elif event.object_type == "Person" %} + {% trans %}Participant {{ name }} added{% endtrans %} + {% endif %} {% elif event.operation_type == OperationType.UPDATE %} - {% if event.object_type == _("Project") %} + {% if event.object_type == "Project" %} {% if event.prop_changed == "password" %} {{ _("Project private code changed") }} {% elif event.prop_changed == "logging_preference" %} {{ change_to_logging_preference(event) }} {% elif event.prop_changed == "name" %} - {{ _("Project renamed to") }} {{ event.val_after }} + {% set new_project_name=event.val_after|em_surround %} + {% trans %}Project renamed to {{ new_project_name }}{% endtrans %} {% elif event.prop_changed == "contact_email" %} - {{ _("Project contact email changed to") }} {{ event.val_after }} + {% set new_email=event.val_after|em_surround %} + {% trans %}Project contact email changed to {{ new_email }}{% endtrans %} {% else %} {{ _("Project settings modified") }} {% endif %} {% elif event.prop_changed == "activated" %} - {{ event.object_type }} {{ event.object_desc }} - {% if event.val_after == False %}{{ _("deactivated") }}{% else %}{{ _("reactivated") }}{% endif %} - {% elif event.prop_changed == "name" or event.prop_changed == "what" %} - {{ describe_object(event) }} {{ _("renamed to") }} {{ event.val_after }} + {% if event.val_after == False %} + {% trans %}Participant {{ name }} deactivated{% endtrans %} + {% else %} + {% trans %}Participant {{ name }} reactivated{% endtrans %} + {% endif %} + {% elif event.prop_changed == "name" %} + {% set new_name=event.val_after|em_surround %} + {% trans %}Participant {{ name }} renamed to {{ new_name }}{% endtrans %} + {% elif event.prop_changed == "what" %} + {% set new_description=event.val_after|em_surround %} + {% trans %}Bill {{ name }} renamed to {{ new_description }}{% endtrans %} {% elif event.prop_changed == "weight" %} - {{ simple_property_change(event, _("Weight")) }} + {% set old_weight=event.val_before|em_surround %} + {% set new_weight=event.val_after|em_surround %} + {% trans %}Participant {{ name }}: weight changed from {{ old_weight }} to {{ new_weight }}{% endtrans %} {% elif event.prop_changed == "external_link" %} - {{ describe_object(event) }}: {{ _("External link changed to") }} - {{ event.val_after }} + {{ bill_property_change(event, _("External link"), None, "{link}".format(link=event.val_after | escape) | safe | em_surround) }} {% elif event.prop_changed == "owers_added" %} {{ owers_changed(event, True)}} {% elif event.prop_changed == "owers_removed" %} {{ owers_changed(event, False)}} {% elif event.prop_changed == "payer" %} - {{ simple_property_change(event, _("Payer"))}} + {{ bill_property_change(event, _("Payer"))}} {% elif event.prop_changed == "amount" %} - {{ simple_property_change(event, _("Amount")) }} + {{ bill_property_change(event, _("Amount")) }} {% elif event.prop_changed == "date" %} - {{ simple_property_change(event, _("Date")) }} + {{ bill_property_change(event, _("Date")) }} {% elif event.prop_changed == "original_currency" %} - {{ simple_property_change(event, _("Currency")) }} + {{ bill_property_change(event, _("Currency")) }} {% elif event.prop_changed == "converted_amount" %} - {{ simple_property_change(event, _("Amount in %(currency)s", currency=g.project.default_currency)) }} + {{ bill_property_change(event, _("Amount in %(currency)s", currency=g.project.default_currency)) }} {% else %} - {{ describe_object(event) }} {{ _("modified") }} + {% if event.object_type == "Bill" %} + {% trans %}Bill {{ name }} modified{% endtrans %} + {% elif event.object_type == "Person" %} + {% trans %}Participant {{ name }} modified{% endtrans %} + {% endif %} {% endif %} {% elif event.operation_type == OperationType.DELETE %} - {{ event.object_type }} {{ event.object_desc }} {{ _("removed") }} + {% if event.object_type == "Bill" %} + {% trans %}Bill {{ name }} removed{% endtrans %} + {% elif event.object_type == "Person" %} + {% trans %}Participant {{ name }} removed{% endtrans %} + {% endif %} {% else %} - {# Should be unreachable #} - {{ describe_object(event) }} {{ _("changed in a unknown way") }} + {# Should be unreachable #} + {% if event.object_type == "Project" %} + {% trans %}Project {{ name }} changed in an unknown way{% endtrans %} + {% elif event.object_type == "Bill" %} + {% trans %}Bill {{ name }} changed in an unknown way{% endtrans %} + {% elif event.object_type == "Person" %} + {% trans %}Participant {{ name }} changed in an unknown way{% endtrans %} + {% endif %} {% endif %}
diff --git a/ihatemoney/tests/history_test.py b/ihatemoney/tests/history_test.py index cee3dca6..1a8d3504 100644 --- a/ihatemoney/tests/history_test.py +++ b/ihatemoney/tests/history_test.py @@ -410,7 +410,7 @@ class HistoryTestCase(IhatemoneyTestCase): self.assertEqual(resp.status_code, 200) self.assertRegex( resp.data.decode("utf-8"), - r"Participant %s:\s* Weight changed\s* from %s\s* to %s" + r"Participant %s:\s* weight changed\s* from %s\s* to %s" % ( em_surround("zorglub", regex_escape=True), em_surround("1.0", regex_escape=True), @@ -426,7 +426,7 @@ class HistoryTestCase(IhatemoneyTestCase): resp.data.decode("utf-8").index( f"Participant {em_surround('zorglub')} renamed" ), - resp.data.decode("utf-8").index("Weight changed"), + resp.data.decode("utf-8").index("weight changed"), ) # delete user using POST method diff --git a/ihatemoney/utils.py b/ihatemoney/utils.py index 6a6d657c..97d94380 100644 --- a/ihatemoney/utils.py +++ b/ihatemoney/utils.py @@ -12,7 +12,7 @@ import socket from babel import Locale from babel.numbers import get_currency_name, get_currency_symbol -from flask import current_app, redirect, render_template +from flask import current_app, escape, redirect, render_template from flask_babel import get_locale, lazy_gettext as _ import jinja2 from werkzeug.routing import HTTPException, RoutingException @@ -300,6 +300,74 @@ class FormEnum(Enum): return str(self.value) +def em_surround(string, regex_escape=False): + # Needed since we're going to assume this is safe later in order to render + # the tag we're adding + string = escape(string) + + if regex_escape: + return fr'{string}<\/em>' + else: + return f'{string}' + + +def localize_list(items, surround_with_em=True): + """ + Localize a list, optionally surrounding each item in tags. + + Uses the appropriate joining character, oxford comma behavior, and handles + 1, and 2 object lists, all according to localizable behavior. + + Examples (using en locale): + >>> localize_list([1,2,3,4,5], False) + 1, 2, 3, 4, and 5 + + >>> localize_list([1,2], False) + 1 and 2 + + Based on the LUA example from: + https://stackoverflow.com/a/58033018 + + :param list: The list of objects to localize by a call to str() + :param surround_with_em: Optionally surround each object with tags + :return: A locally formatted list of objects + """ + + if len(items) == 0: + return "" + + item_wrapper = em_surround if surround_with_em else lambda x: x + wrapped_items = list(map(item_wrapper, items)) + + if len(wrapped_items) == 1: + return str(wrapped_items[0]) + elif len(wrapped_items) == 2: + # I18N: List with two items only + return _("{dual_object_0} and {dual_object_1}").format( + dual_object_0=wrapped_items[0], dual_object_1=wrapped_items[1] + ) + else: + # I18N: Last two items of a list with more than 3 items + output_str = _("{previous_object}, and {end_object}").format( + previous_object="{previous_object}", end_object=wrapped_items.pop() + ) + # I18N: Two items in a middle of a list with more than 5 objects + middle = _("{previous_object}, {next_object}") + while len(wrapped_items) > 2: + temp = middle.format( + previous_object="{previous_object}", + next_object=wrapped_items.pop(), + ) + output_str = output_str.format(previous_object=temp) + + output_str = output_str.format(previous_object=wrapped_items.pop()) + # I18N: First two items of a list with more than 3 items + output_str = _("{start_object}, {next_object}").format( + start_object="{start_object}", next_object=output_str + ) + return output_str.format(start_object=wrapped_items.pop()) + + def render_localized_currency(code, detailed=True): if code == "XXX": return _("No Currency")