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

Removes redundant call to apiGetQuestionSet from scorescreen. #1450

Conversation

cayb0rg
Copy link
Contributor

@cayb0rg cayb0rg commented Feb 21, 2023

This issue was raised within Guess The Phrase. I believe this may solve potential expiration issues, since the call to function question_set_get will sometimes cause the score screen to expire unnecessarily if the user is not authenticated. apiGetWidgetInstance has a built-in option to fetch the qset already, so I've made the score screen use that instead.

Copy link
Member

@clpetersonucf clpetersonucf left a comment

Choose a reason for hiding this comment

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

This looks good and does the job as expected - the only thing I'd like to see, and only because we have tests for the widget_instances_get API call already already, is to add additional tests for non-authenticated users requesting specific instance information, and to test the $load_qset flag.

@clpetersonucf clpetersonucf added the React Branch Related to the React rewrite for Materia label Mar 22, 2023
Copy link
Member

@clpetersonucf clpetersonucf left a comment

Choose a reason for hiding this comment

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

This does what's needed - looking back at the big picture, allowing score screens to be viewed without being logged in AND supporting custom score screens with access to the qset makes for a tricky situation, but definitely a bigger one than what this PR was designed to solve. We might revisit it in the future, but this will serve for now.

@clpetersonucf clpetersonucf merged commit f41e2e7 into ucfopen:issue/support-dashboard-in-react Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
React Branch Related to the React rewrite for Materia
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants