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

Guest user lti1p3 lang detection #365

Closed
wants to merge 1 commit into from

Conversation

poyuki
Copy link
Contributor

@poyuki poyuki commented Apr 21, 2023

REL-1101

Summary

This PR aim to provide lang detection for guest user and usage of DEFAULT_ANONYMOUS_INTERFACE_LANG.

I have some doubts about this solution. From my point of view, it's a bit crutchy, but at the moment I couldn't find anything more suitable.

Demo

https://oat-sa.atlassian.net/browse/REL-1101?focusedCommentId=225078

How to test

  • checkout branch
  • update DEFAULT_ANONYMOUS_INTERFACE_LANG
  • launch remote delivery on TAO 3.x
  • defined locale should be used in Advance testrunner

@poyuki poyuki force-pushed the bug/REL-1101/lti1p3-guest-lang branch 3 times, most recently from 6dabce1 to 8319934 Compare April 21, 2023 12:06
@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (02dea2b) 31.62% compared to head (dd53a26) 31.00%.
Report is 99 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop     #365      +/-   ##
=============================================
- Coverage      31.62%   31.00%   -0.62%     
- Complexity       803      914     +111     
=============================================
  Files            102      106       +4     
  Lines           2726     3151     +425     
=============================================
+ Hits             862      977     +115     
- Misses          1864     2174     +310     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@poyuki poyuki force-pushed the bug/REL-1101/lti1p3-guest-lang branch from 8319934 to dd53a26 Compare April 21, 2023 12:11
Copy link
Member

@wazelin wazelin left a comment

Choose a reason for hiding this comment

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

Right. This does work quite well, but not exactly what I had in mind.

I believe the change should be done on the generis-user itself.

oat-sa/generis#1037 can be another way to solve this problem.

@poyuki
Copy link
Contributor Author

poyuki commented Apr 21, 2023

@wazelin I just moved in 0 breaking change direction

@wazelin
Copy link
Member

wazelin commented Apr 21, 2023

@wazelin I just moved in 0 breaking change direction

Fair enough.
Let's return to this problem and options to its solution on Monday. Both options are viable IMO. We just have to understand better what the trade offs of each are.

You being away next week shouldn't be a blocker for us. I'll discuss this with @augustas.

Thank you for this solution!

@poyuki
Copy link
Contributor Author

poyuki commented May 2, 2023

closed in favor of oat-sa/generis#1037

@poyuki poyuki closed this May 2, 2023
@wazelin wazelin reopened this Feb 15, 2024
Copy link

Version

Target Version 15.15.1
Last version 15.15.0

There are 0 BREAKING CHANGE, 0 feature, 1 fix

@wazelin
Copy link
Member

wazelin commented Feb 15, 2024

Reviving the PR with a dependency on oat-sa/generis#1106.

Your 0-breaking-change approach was the way to go after all, @poyuki. More info under https://oat-sa.atlassian.net/browse/INF-258

@wazelin wazelin closed this Apr 3, 2024
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.

3 participants