Skip to content
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.

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

Already on GitHub? Sign in to your account

use helpers to i18nize configured permission levels/options #4298

Merged
merged 1 commit into from
May 12, 2020

Conversation

no-reply
Copy link
Contributor

@no-reply no-reply commented May 6, 2020

these configured options (why do they need to be configurable? how would one
configure them successfully?) need to be i18nized, but the current fix only
caches the i18n strings at app startup (or maybe when the configurations are
first hit?).

to translate on-the-fly we need to hit the i18n key while the user is in the
desired locale.

this is a stop-gap fix. probably we want to make the configuration options just
straight arrays, or remove them completely. discussion on longer term approachs
is scheduled in https://wiki.lyrasis.org/display/samvera/Samvera+Tech+Call+2020-06-05.

related to #4297.

@samvera/hyrax-code-reviewers

Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

I believe this isn't backwards compatible if anyone has been customizing these by doing something like Hyrax.config.permission_levels = {} in an initializer.
I don't know what the use case would be for customizing the values and I think replacing customizable keys with i18n locale strings is the best long-term approach. So deprecate in 2.x and make the switch in 3.0?

@no-reply
Copy link
Contributor Author

no-reply commented May 6, 2020

I believe this isn't backwards compatible if anyone has been customizing these by doing something like Hyrax.config.permission_levels = {} in an initializer.

why not? the configs are still used by the helpers.


to clarify, i think in the current implementation, Hyrax.config.permission_levels = {} will work fine; this would lead the associated helper to also return an empty hash.

Hyrax.config.permission_levels = { 'Hamburger' => 'edit', 'Lettuce' => 'read' } won't work, in the sense that "Hamburger" and "Lettuce" would be ignored completely and replaced by the i18nized values. i think this latter part is a feature, rather than a bug, and could just go in the 3.0.0 release notes.

@no-reply no-reply requested a review from elrayle May 6, 2020 17:19
@no-reply no-reply force-pushed the i18nize_levels branch 2 times, most recently from 1dcb6ff to 3e7cbf7 Compare May 6, 2020 17:28
these configured options (why do they need to be configurable? how would one
configure them successfully?) need to be i18nized, but the current fix only
caches the i18n strings at app startup (or maybe when the configurations are
first hit?).

to translate on-the-fly we need to hit the i18n key while the user is in the
desired locale.

this is a stop-gap fix. probably we want to make the configuration options just
straight arrays, or remove them completely. discussion on longer term approachs
is scheduled in https://wiki.lyrasis.org/display/samvera/Samvera+Tech+Call+2020-06-05.
Copy link
Contributor

@bkeese bkeese left a comment

Choose a reason for hiding this comment

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

👍

@no-reply no-reply dismissed elrayle’s stale review May 12, 2020 16:32

made these changes

@no-reply no-reply merged commit 76c95c3 into master May 12, 2020
@no-reply no-reply deleted the i18nize_levels branch May 12, 2020 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants