fix: return translated role definitions#11466
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue of always returning role definitions in English by adding localization support based on the Accept-Language header.
- Adds translation logic for role definitions in both the GetRoleDefinitions and GetRoleDefinition endpoints.
- Updates the changelog to document the bugfix.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| services/graph/pkg/service/v0/rolemanagement.go | Adds localization support for role definitions by translating the fields based on the Accept-Language header. |
| changelog/unreleased/fix-return-translated-role-definitions.md | Documents the bugfix to return translated role definitions. |
Comments suppressed due to low confidence (1)
services/graph/pkg/service/v0/rolemanagement.go:59
- [nitpick] In the GetRoleDefinition function, consider updating the error message to reflect that it's handling a single role definition rather than multiple, for improved clarity in logs.
logger.Error().Err(err).Msg("could not translate role definitions")
| locale := r.Header.Get(l10n.HeaderAcceptLanguage) | ||
| if locale != "" && locale != "en" { | ||
| err := l10n_pkg.TranslateEntity(locale, "en", roles, | ||
| l10n.TranslateField("DisplayName"), | ||
| l10n.TranslateField("Description"), | ||
| ) | ||
| if err != nil { | ||
| logger.Error().Err(err).Msg("could not translate role definitions") | ||
| } | ||
| } |
There was a problem hiding this comment.
Shouldn't we use something like
userID := revactx.ContextMustGetUser(ctx).GetUserId()
locale := l10n.MustGetUserLocale(r.Context(), userID, r.Header.Get(l10n.HeaderAcceptLanguage), g.valueService)This would also take the users language setting into account when the header is not set.
There was a problem hiding this comment.
Shouldn't this endpoint be user independent? Why would we authenticate the user to get a definition?
There was a problem hiding this comment.
Cleared out in a call - it's a private endpoint so we should use the settings.
There was a problem hiding this comment.
@kobergj I adjusted the rolemanagement to use the users language setting and I also added it into the drive API. I had to extend the DriveItemPermissionsApi to get the ValueService in there.
656e35f to
ca057b5
Compare
Instead of always returning the role definitions in English, we now return the role definitions in the language set in the header if present.
ca057b5 to
6c102b4
Compare
|
…ions fix: return translated role definitions
Description
Instead of always returning the role definitions in English, we now return the role definitions in the language set in the header if present.
Motivation and Context
Content is delivered to users in their preferred locale.
How Has This Been Tested?
deAccept-LanguageheaderTypes of changes