fix: Failing invenio-admin due to lazy string

[mesemus]

Description

Lazy strings are not handled correctly in column_labels part of flask-admin's ModelAdmin class (and seem to be ok elsewhere). This PR fixes that by converting column_labels to computed properties and performing simple gettext there.

This bug was reported in #529

After the fix the UI looks like:

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

• I'm aware of the code of conduct.
• I've created logical separate commits and followed the commit message format.
• I've added relevant test cases.
• I've added relevant documentation.
• I've marked translation strings.
• I've identified the copyright holder(s) and updated copyright headers for touched files (>15 lines contributions).
• I've NOT included third-party code (copy/pasted source code or new dependencies).
  • If you have added third-party code (copy/pasted or new dependencies), please reach out to an architect on Discord.

Frontend

• I've followed the CSS/JS and React guidelines.
• I've followed the web accessibility guidelines.
• I've followed the user interface guidelines.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

1. You license your contribution under the same terms as the current repository’s license.
2. You agree that you have the right to license your contribution under the current repository’s license.

[Samk13]

Thanks for fixing this! Using _() instead of lazy_gettext() makes sense.
Quick question: by turning column_labels into a read-only @property, don’t we break the common pattern of overriding view.column_labels = {...} in instances/subclasses?

Otherwise LGTM!

[mesemus]

You are right, if someone was inheriting from the class their code would be broken.

As it is in session activity view and user identity view i guess it is unlikely that someone would do so - but ofc I can't be sure.

An alternative (apart from patching flask admin) would be not to have translations for this view (it is more or less deprecated so may make sense).

Thanks for fixing this! Using _() instead of lazy_gettext() makes sense.
Quick question: by turning column_labels into a read-only @property, don’t we break the common pattern of overriding view.column_labels = {...} in instances/subclasses?

Otherwise LGTM!

[utnapischtim]

this change does only affects the admin page or?
this would mean that InvenioRDM is not affected by this change?

[mesemus]

Yes, this is the old flask-admin implementation which is not used in InvenioRDM anymore, so RDM is not affected.

The RDM implementation of user administration is directly in invenio_app_rdm and does not use this code.

[Samk13]

Given that context (and since these views are more or less deprecated), maybe the simplest/least risky fix is just to drop the translations here instead of adding the property override logic?
That way we avoid breaking overrides and keep the code minimal. What do you think @mesemus @utnapischtim ?

[zzacharo]

We still use this module in some services at CERN e.g. https://github.com/CERNDocumentServer/cds-videos . Checking quickly our code we do override the column labels dict but not for the SessionActivityView or UserIdentityView so it is fine from our side... Can we make the change backwards compatible e.g. implement a dict like class?

[mesemus]

"LazyTranslationDict" would not raise Exception in the Child class, but would not provide working translation as well. Let's have:

class LazyTranslationDict(dict):
   def __getitem__(): str(super().__getitem__())
   def values(): (str(x) for x in super().values())
   def items(): ...

class Parent:
    column_labels = LazyTranslationDict({"key": lazy_gettext("...")})

The common way of extending is with **:

from invenio_i18n import gettext as _

class Child(Parent):
    column_labels = {**Parent.column_labels, "mykey": _("incorrect translation")}

If we do this, the values would get converted to plain string at the time of initialization, not at the time of request. That means that the UI would get the translation for the locale of the server, not the locale of the actual request.

With property, the same child code will fail at the time of initialization:

class Parent:
   @property
   def column_labels(self): ...

and would force fixing the "Child" class. The fix would be to convert it to property as well, and then the translation for "mykey" would start to work.

@jma @zzacharo do you plan to have translations in your own admin classes? If you plan to migrate to RDM & new administration and do not need to have functioning translation of /admin, then I would say let's just remove the lazy_gettext from here and keep it untranslated.

[zzacharo]

In our case, we have only english translation so it is not a problem... That said, we are also installing invenio-accounts==5.1.7 so maybe it is also ok the "breaking change" as part of the v6.0.0 major release.... given the fact that invenio-admin is somewhat deprecated for most of the instances... But indeed @jma how it is impacting you?