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

Fix inconsistency between Pass Required and Passing Grade for answer feedback #7525

Merged
merged 8 commits into from Mar 7, 2024

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Mar 6, 2024

Related with #7524

Proposed Changes

It's a little complicated to explain. So to understand the context, I'd like you to use trunk and create a new course with a quiz, following the normal flow (through the Course Outline) and only turning on the setting "Auto Grade" on the quiz. Add to the quiz a multiple choice question with an answer feedback.

Answer the quiz as a student, and check that you will see the answer feedback. So I think we could consider it as the default behavior of a normal user, right?

The course creators could see different behaviors if they check the "Pass Required" setting, change the related settings, and turn it off.

The current behavior also has some issues like displaying the answer feedback wrapper and not displaying the content of it. See it when changing the passing grade to 100, unchecking the "Pass Required", and grading the quiz with wrong answers (not achieving the configured passing grade).

This PR fixes this inconsistency with the answers feedback by always displaying it if the "Pass Required" is turned off.

Cons: Users that changed the settings that are displayed when "Pass Required" is checked, and then turn it off again might see a behavior change depending on their settings.

For more context, see this:

if ( ! Sensei_Quiz::is_pass_required( $lesson_id ) || $user_passed || ! Sensei_Quiz::is_reset_allowed( $lesson_id ) ) {
$show_answers = true;
}

I suspect the pass required check was missed as part of this refactor: #4408

Testing Instructions

  1. Checkout this branch.
  2. Create a course with a lesson and a quiz.
  3. Add at least a question with answer feedback.
  4. In the quiz settings, check the option "Pass Required".
  5. Change the Passing Grade to "100" and make sure the 3 toggles for Answer Feedback are unchecked.
  6. Now uncheck the "Pass Required" and publish the lesson.
  7. As a student, navigate to the quiz and answer it.
  8. Make sure you see the answer feedback.

Pre-Merge Checklist

  • PR title and description contain sufficient detail and accurately describe the changes
  • Acceptance criteria is met
  • Decisions are publicly documented
  • Adheres to coding standards (PHP, JavaScript, CSS, HTML)
  • All strings are translatable (without concatenation, handles plurals)
  • Follows our naming conventions (P6rkRX-4oA-p2)
  • Hooks (p6rkRX-1uS-p2) and functions are documented
  • New UIs are responsive and use a mobile-first approach
  • New UIs match the designs
  • Different user privileges (admin, teacher, subscriber) are tested as appropriate
  • Legacy courses (course without blocks) are tested
  • Code is tested on the minimum supported PHP and WordPress versions
  • User interface changes have been tested on the latest versions of Chrome, Firefox and Safari
  • "Needs Documentation" label is added if this change requires updates to documentation
  • Known issues are created as new GitHub issues

Copy link

github-actions bot commented Mar 6, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 93.54839% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 51.79%. Comparing base (229d03b) to head (8281657).
Report is 5 commits behind head on trunk.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7525      +/-   ##
============================================
+ Coverage     51.57%   51.79%   +0.22%     
- Complexity    11235    11239       +4     
============================================
  Files           622      622              
  Lines         47518    47520       +2     
  Branches        414      414              
============================================
+ Hits          24506    24613     +107     
+ Misses        22679    22574     -105     
  Partials        333      333              
Files Coverage Δ
includes/class-sensei-question.php 23.01% <93.54%> (+11.75%) ⬆️

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 735df53...8281657. Read the comment docs.

@@ -923,7 +923,7 @@ public static function the_answer_feedback( $question_id ) {
$quiz_graded = $user_quiz_progress && ! in_array( $user_quiz_progress->get_status(), array( 'ungraded', 'in-progress' ) );

$quiz_required_pass_grade = intval( get_post_meta( $quiz_id, '_quiz_passmark', true ) );
$succeeded = $user_quiz_grade >= $quiz_required_pass_grade;
$succeeded = ! Sensei_Quiz::is_pass_required( $lesson_id ) || $user_quiz_grade >= $quiz_required_pass_grade;
Copy link
Contributor Author

@renatho renatho Mar 6, 2024

Choose a reason for hiding this comment

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

In the editor UI, we hide the "Passing Grade" if the "Pass Required" is turned off. So the behavior in the frontend should be the same ("Passing Grade" shouldn't matter if "Pass Required" is turned off).

In this case, we are considering the quiz as succeeded if "Pass Required" is turned off for the answer feedback logic.

Copy link

github-actions bot commented Mar 6, 2024

Test the previous changes of this PR with WordPress Playground.

@renatho renatho changed the title Don't show answer feedback wrapper when feedback will not be displayed Fix inconsistency between Pass Required and Passing Grade for answer feedback Mar 6, 2024
Copy link

github-actions bot commented Mar 6, 2024

Test the previous changes of this PR with WordPress Playground.

</div>
<?php } ?>
</div>
<?php if ( $indicate_incorrect || $has_answer_notes || $correct_answer ) { ?>
Copy link
Contributor Author

@renatho renatho Mar 6, 2024

Choose a reason for hiding this comment

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

The only change here was this extra conditional to not display the wrapper when not needed. The rest is only indentation.

After 44e491c, I don't see any logic that would need this conditional, but I'll keep it only for safety given the logic of this method.

@renatho renatho self-assigned this Mar 6, 2024
Copy link

github-actions bot commented Mar 6, 2024

Test the previous changes of this PR with WordPress Playground.

@renatho renatho added this to the 4.21.1 milestone Mar 6, 2024
Copy link

github-actions bot commented Mar 6, 2024

Test the previous changes of this PR with WordPress Playground.

@renatho renatho requested a review from a team March 6, 2024 16:29
@renatho renatho marked this pull request as ready for review March 6, 2024 16:29
Copy link

github-actions bot commented Mar 6, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Mar 6, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link
Member

@m1r0 m1r0 left a comment

Choose a reason for hiding this comment

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

Works as described! Thanks for fixing this.

@renatho renatho merged commit 4442766 into trunk Mar 7, 2024
24 checks passed
@renatho renatho deleted the fix/answer-feedback branch March 7, 2024 14:26
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.

None yet

2 participants