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

Show teacher's questions only when using category questions #4148

Merged
merged 8 commits into from Apr 5, 2021

Conversation

gikaragia
Copy link
Contributor

@gikaragia gikaragia commented Mar 31, 2021

Overview

In the quiz editor, when a category question was added, all questions were included regardless if they belong to the teacher or not. With this change this is now fixed.

Changes proposed in this Pull Request

  • If the quiz author is not admin, then only the questions that he owns are displayed when a category question is expanded.
  • If the quiz author is an admin all questions are added since as an admin he has access to all questions.
  • In the quiz editor, the question counts have been updated in order for the warning to displayed correctly.
  • While I was searching the code, I also did some cleanup.
  • @donnapep since this is going to reduce the questions that a quiz has in some of the cases, we might want to mention it in the next release post. Users can use the sensei_filter_category_questions_by_author to restore the previous behaviour.

Testing instructions

  • With a teacher, create some questions and add them to a category.
  • Do the same with another teacher or with an admin.
  • Login with the first teacher and try adding a question category to a lesson's quiz.
  • Observe that only the questions that the teacher owns are taken into account.
  • Observe in the frontend and in grading that the correct questions are included.
  • Repeat the same with an admin and observe that all questions are included.

New Filter

  • sensei_filter_category_questions_by_author - Whether category questions should be filtered by author.

Deprecated Functions

  • Sensei_Frontend::sensei_get_user_quiz_answers - Use Sensei_Quiz::get_user_answers instead.
  • Sensei_Quiz::stop_quiz_questions_loop - No replacement.

@gikaragia gikaragia requested a review from a team March 31, 2021 12:18
@gikaragia gikaragia requested a review from jom April 1, 2021 06:58
@yscik
Copy link
Contributor

yscik commented Apr 1, 2021

I wonder if it's a legit use case that teachers want to share questions with each other. It works currently, right? Would it be safer to keep allowing it by default, and have the filter (or possibly a setting) to disable it?

Copy link
Member

@jom jom left a comment

Choose a reason for hiding this comment

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

Tested this and it works really well! I also tried switching to this branch when the learner has a saved quiz but hasn't submitted it and it doesn't remove the questions (which is good, I think).

One tiny doc change and then I'm ready to approve.

@yscik Before teachers weren't allowed to add category questions (just if an admin transferred an admin course to a teacher). I think it is safer to assume not so other teacher's questions don't start appearing on their quizzes.

@gikaragia gikaragia added this to the 3.10.0 milestone Apr 2, 2021
@gikaragia gikaragia added the Hooks This change adds or modifies one or more hooks. label Apr 2, 2021
Co-authored-by: Jake Oehler Morrison <jake.morrison@automattic.com>
@gikaragia
Copy link
Contributor Author

@yscik just wanted to add that I think that the main issue with the current way this works is that while questions belonging to another teacher can be included in the quiz, the quiz owner cannot access these questions in admin.

@gikaragia gikaragia requested a review from jom April 2, 2021 10:42
@gikaragia gikaragia merged commit 9b3ab7f into master Apr 5, 2021
@donnapep donnapep deleted the add/count-teachers-questions-only branch April 12, 2021 12:17
@donnapep donnapep added the Deprecation This change introduces a deprecation. label Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecation This change introduces a deprecation. Hooks This change adds or modifies one or more hooks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants