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 timed quiz still working for students even after time ends #7675

Merged
merged 14 commits into from
Nov 7, 2024

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Sep 4, 2024

Resolves https://github.com/Automattic/sensei-pro/issues/2606

Proposed Changes

In timed quizzes, when time is up, the frontend code clicks the completion button to submit the quiz. But in the cases where the quiz was paginated, completion button only got rendered on the last page. So if the student was not on the last page, the quiz didn't get submitted, and the student was still able to answer.

To fix that, we're rendering the button even in the middle pages so that the completion function still works, but keeping it hidden in those cases so that UI doesn't break or change from before.

Testing Instructions

  1. Create a Course with a lesson that has a quiz with many questions
  2. Make sure you have Sensei Pro enabled
  3. From the quiz settings, set pagination to Multi Page and set per page value to 1 or a small number to generate many pages
  4. Also enable Quiz Timer option, set a small time
  5. Now take the quiz as a student, be on any page other than the last page
  6. Make sure when time is up, the quiz gets submitted
  7. Now take the quiz as another student, finish the quiz normally and make sure it works
  8. Now take the quiz as a student for the third time, stay on the last page of the quiz without submitting, make sure the quiz gets submitted automatically when the time is up.

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

@Imran92 Imran92 added this to the 4.24.4 milestone Sep 4, 2024
@Imran92 Imran92 requested a review from a team September 4, 2024 07:19
@Imran92 Imran92 self-assigned this Sep 4, 2024
Copy link

github-actions bot commented Sep 4, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Sep 4, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.37%. Comparing base (f13414e) to head (1708578).
Report is 110 commits behind head on trunk.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7675      +/-   ##
============================================
+ Coverage     51.92%   52.37%   +0.44%     
- Complexity    11449    11463      +14     
============================================
  Files           651      653       +2     
  Lines         48659    48761     +102     
  Branches        468      480      +12     
============================================
+ Hits          25268    25537     +269     
+ Misses        23012    22835     -177     
- Partials        379      389      +10     

see 9 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 ca19e19...1708578. Read the comment docs.

Copy link

github-actions bot commented Sep 4, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Sep 4, 2024

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Sep 4, 2024

Test the previous changes of this PR with WordPress Playground.

@@ -403,6 +403,10 @@ a.sensei-certificate-link {
}
}

.sensei-item-no-display {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be following the BEM standards here and use a modifier to indicate the state change instead of adding a new class (i.e. --hidden). Hopefully we can put this CSS in a file that only loads for quizzes instead of adding it to this catch-all file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @donnapep ! I've implemented the changes here f920701 .

Initially I used a more generic class and placed it in a catch-all file because I was envisioning it a bit like bootstrap or tailwind's general purpose class to hide an element. To avoid having display: none for many different classes in many files. But I've changed it now.

An additional note is that we didn't have any specific stylesheet that loads only for single Quizzes. So I've added one, and adding this style only there. Styles for quizzes used to go into some catch-all files. I haven't moved those quiz specific styles in this PR to avoid risk of breaking and also that'd be out of scope for this PR. But moving the quiz specific styles from general stylesheet to this new quiz specific stylesheet will probably be a good idea once we are done with bugs and important enhancements. Should I create an issue for that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I create an issue for that?

Sure!

@donnapep
Copy link
Collaborator

@Imran92 Can you please address the feedback in this one? 🙇🏻‍♀️

Copy link

Test the previous changes of this PR with WordPress Playground.

Copy link

github-actions bot commented Oct 30, 2024

WordPress Dependencies Report

The github-action-wordpress-dependencies-report action has detected some script changes between the commit b54aec3 and trunk. Please review and confirm the following are correct before merging.

Script Handle Added Dependencies Removed Dependencies Total Size Size Diff
blocks/quiz/style.js wp-polyfill 24 B +24 B ( +100% 🔼 )

This comment was automatically generated by the github-action-wordpress-dependencies-report action.

Copy link

Test the previous changes of this PR with WordPress Playground.

Copy link

Test the previous changes of this PR with WordPress Playground.

@Imran92
Copy link
Contributor Author

Imran92 commented Oct 30, 2024

@Imran92 Can you please address the feedback in this one? 🙇🏻‍♀️

Thanks Donna! I've updated it here #7675 (comment)

Copy link

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.

Looks good to me and works as described!

I think having the quiz CSS in a separate file is a good idea so 👍. Thanks for the tests as well!

includes/class-sensei-quiz.php Show resolved Hide resolved
@Imran92 Imran92 merged commit 745f967 into trunk Nov 7, 2024
22 checks passed
@Imran92 Imran92 deleted the fix/timed-quiz-input-not-stopping-when-time-ends branch November 7, 2024 19:49
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