Skip to content

fix: Failing invenio-admin due to lazy string #531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mesemus wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Failing invenio-admin due to lazy string #531

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 16 additions & 9 deletions invenio_accounts/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,15 @@ class SessionActivityView(ModelView):

list_all = ("user.id", "user.email", "sid_s", "created")

column_labels = {
"user.id": lazy_gettext("User ID"),
"user.email": lazy_gettext("Email"),
"sid_s": lazy_gettext("Session ID"),
}
@property
def column_labels(self):

@utnapischtim utnapischtim Aug 26, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mesemus mesemus Aug 26, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Samk13 Aug 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 zzacharo Aug 27, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mesemus Aug 27, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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 zzacharo Aug 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

"""Return translated column labels."""
return {
"user.id": _("User ID"),
"user.email": _("Email"),
"sid_s": _("Session ID"),
}

column_list = list_all
column_filters = list_all
column_sortable_list = list_all
Expand Down Expand Up @@ -218,10 +222,13 @@ class UserIdentityView(ModelView):
"user.email",
)

column_labels = {
"user.email": lazy_gettext("Email"),
"id_user": lazy_gettext("User ID"),
}
@property
def column_labels(self):
"""Return translated column labels."""
return {
"user.email": _("Email"),
"id_user": _("User ID"),
}


session_adminview = {
Expand Down
Loading