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

StudentQuiz: Student can view and attempt the questions that belong t… #459

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

BruceGoodGuy
Copy link
Contributor

@BruceGoodGuy BruceGoodGuy commented Jul 31, 2023

Hi @timhunt ,

I have resolved this ticket by updating the function require_access_to_a_relevant_group. After spending time investigating this issue, I realized that the function require_access_to_a_relevant_group only covers cases where the user doesn't have the current group and tries to access the student quiz with the group mode set to separate groups.

However, in situations where the user has another group that does not belong to the valid group, this function does not handle it properly. Therefore, I believe it's necessary to update this function to address this scenario.

Regarding the use of exit() in this function, it makes it challenging to write unit tests for it when it always stop the code flow, so that I can't write Unite test for it.
Commit ID: 4cbcd6d8b267e83f7ff29616fa2df3a08aeb0d94
I'd appreciate your feedback on my changes. Please let me know your thoughts. Thank you.

@BruceGoodGuy BruceGoodGuy force-pushed the wip703356 branch 3 times, most recently from efac1d4 to 4cbcd6d Compare August 7, 2023 07:36
@timhunt
Copy link
Member

timhunt commented Aug 9, 2023

Sorry, but groups_get_user_groups is not the right function to use. We don't want course groups here, just the ones relevant to this activity, so it should be groups_get_activity_allowed_groups.

Except that, should not be implementing this logic yourself if you don't need to, and I think groups_group_visible does exactly what you need.

@timhunt timhunt merged commit 7891b64 into studentquiz:main Aug 18, 2023
3 of 4 checks passed
@timhunt
Copy link
Member

timhunt commented Aug 18, 2023

Thanks. That makes sense now. Merging.

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.

2 participants