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

feat: add a setting flag to control user pref lang #5

Closed

Conversation

johanseto
Copy link
Collaborator

@johanseto johanseto commented Jan 22, 2024

Description

This is migration issue.

This add a setting variable that allow to show if the LANGUAGE_CODE of the site config would have more priority than the cookie of the user preference. The setting is IGNORE_USER_LANGUAGE_COOKIE, so by default the site config LANGUAGE_CODE woul have more prior but if the setting is set to False the user cookie would work.

migration context
The commit of the migration could not be applied due openedx change dark_lang middleware and set by default site_config language_code priority over the cookie. You can check that the function _set_site_or_microsite_language was removed.

Testing instructions

Use eox-tenant and set LANGUAGE_CODE. you can see there the cookie would be ignored.

Before

Screencast.from.22-01-24.16.57.30.webm

After

Screencast.from.22-01-24.17.07.23.webm

Supporting information

PRS related.
eduNEXT/edunext-platform#672

This add a setting variable that allow to show if the LANGUAGE_CODE of the site config
would have more priority than the cookie of the user preference.
The setting is `IGNORE_USER_LANGUAGE_COOKIE`, so by default the site config `LANGUAGE_CODE`
woul have more prior but if the setting is set to False the user cookie would work.
@andrey-canon
Copy link
Collaborator

here you are changing the migration commit logic and adding a new feature and I'd like to know why, is there another issue that you want to cover ?

@johanseto
Copy link
Collaborator Author

johanseto commented Jan 23, 2024

here you are changing the migration commit logic and adding a new feature and I'd like to know why, is there another issue that you want to cover ?

I had to change the migration commit logic due the code was moved, and the function _set_site_or_microsite_language was removed.
The "feature" is adding a setting with the idea only to try move it to upstream. I think that maybe more people would like to select when to prior the cookie user preference than the siteconfiguration forced language.

Maybe the setting could be renamed to USE_SITE_CONFIG_OVER_USER_LANGUAGE_COOKIE or something like that

@andrey-canon
Copy link
Collaborator

with logic a don't refer with the place where the code is writing I refer to what happens under some specific circumstances.

Current maple logic:
if the cookie doesn't exist and LANGUAGE_CODE exists the cookie is set with the value of LANGUAGE_CODE otherwise the cookie value will be used

This PR
if the cookie doesn't exist, LANGUAGE_CODE exists and IGNORE_USER_LANGUAGE_COOKIE is True the cookie is set with the value of LANGUAGE_CODE , however if the cookie doesn't exist, LANGUAGE_CODE exists and IGNORE_USER_LANGUAGE_COOKIE is False(since someone consider more important the user preference) the cookie is not set, I performed some tests and last case doesn't affect the page since the language is set to the value of LANGUAGE_CODE however that could affect frontend features that depend on that specific cookie (probably dga language selector)

Finally since we are using eox-tenant the settings behavior is different this code was designed with two different sources, first one is django settings, second one is site configuration, if LANGUAGE_CODE is set in the django settings the whole instance will use that as default value and the user preference will have the priority but if LANGUAGE_CODE is set in the site configuration this will override the django setting and the user preference, so to include the setting in the site configuration could be equivalent to IGNORE_USER_LANGUAGE_COOKIE = True on the other hand if you don't include it that could be equivalent to IGNORE_USER_LANGUAGE_COOKIE = False; in the eox-tenant case is different since there is only one source that is the django settings

@johanseto
Copy link
Collaborator Author

johanseto commented Jan 26, 2024

with logic a don't refer with the place where the code is writing I refer to what happens under some specific circumstances.

Current maple logic: if the cookie doesn't exist and LANGUAGE_CODE exists the cookie is set with the value of LANGUAGE_CODE otherwise the cookie value will be used

This PR if the cookie doesn't exist, LANGUAGE_CODE exists and IGNORE_USER_LANGUAGE_COOKIE is True the cookie is set with the value of LANGUAGE_CODE , however if the cookie doesn't exist, LANGUAGE_CODE exists and IGNORE_USER_LANGUAGE_COOKIE is False(since someone consider more important the user preference) the cookie is not set, I performed some tests and last case doesn't affect the page since the language is set to the value of LANGUAGE_CODE however that could affect frontend features that depend on that specific cookie (probably dga language selector)

Finally since we are using eox-tenant the settings behavior is different this code was designed with two different sources, first one is django settings, second one is site configuration, if LANGUAGE_CODE is set in the django settings the whole instance will use that as default value and the user preference will have the priority but if LANGUAGE_CODE is set in the site configuration this will override the django setting and the user preference, so to include the setting in the site configuration could be equivalent to IGNORE_USER_LANGUAGE_COOKIE = True on the other hand if you don't include it that could be equivalent to IGNORE_USER_LANGUAGE_COOKIE = False; in the eox-tenant case is different since there is only one source that is the django settings

I understood you. Instead of adding that setting flag IGNORE_USER_LANGUAGE_COOKIE, what do you think in this commit 159ac19. With that the migration logic is keeped the same and I don't add new feature.

logic:
if the cookie doesn't exist and LANGUAGE_CODE exists the cookie is set with the value of LANGUAGE_CODE otherwise the cookie value will be used

using LANGUAGE_CODE = 'AR' setting:
Peek 2024-01-26 11-49

@johanseto
Copy link
Collaborator Author

@andrey-canon looking for the tests like test_site_language_ignores_user_preferences, I am thinking if it is not better to move this feature to custom middleware in eox-nelp to use the nelp cases??

Or is better to update that test test_site_language_ignores_user_preferences

@andrey-canon
Copy link
Collaborator

@andrey-canon looking for the tests like test_site_language_ignores_user_preferences, I am thinking if it is not better to move this feature to custom middleware in eox-nelp to use the nelp cases??

Or is better to update that test test_site_language_ignores_user_preferences

I think it's better to move out this is my suggestion:

class NewMiddleware(LanguagePreferenceMiddleware):

    def process_request(self, request):
        language_cookie = request.COOKIES.get(settings.LANGUAGE_COOKIE_NAME, None)
   
        if language_cookie:
            ## Since eox_tenant use the settings as the site configuration values, this should be enough 
            setattr(settings, "LANGUAGE_CODE", language_cookie)

        return super().process_request(request)

should we restore the previous value ?
Please test it a let me know if that covers our requirements

@johanseto
Copy link
Collaborator Author

This PR is closed in favor of continuing with the approach of an external plugin fix.
eduNEXT/eox-nelp#127

@johanseto johanseto closed this Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants