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.

Sign up for GitHub

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

Jump to bottom

Fix empty string check in quiz question answers #7614

Merged
merged 4 commits into from
Jun 12, 2024
Merged

Fix empty string check in quiz question answers #7614

merged 4 commits into from
Jun 12, 2024

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Jun 10, 2024 •
edited
Loading

Resolves #7606

Proposed Changes

  • Fix empty string check in quiz question answers.

Testing Instructions

  1. Course with a lesson and a quiz.
  2. Add a multiple choice question.
  3. Add an answer with the text 0.
  4. Save the lesson and make sure the answer wasn't removed.
  5. Access the quiz as a student, and make sure it works properly.

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

@renatho
Fix empty string check
8b3bca9 
It was failing with falsy values
@renatho
Add changelog
85a6a05
@renatho renatho self-assigned this Jun 10, 2024
@renatho renatho added this to the 4.24.1 milestone Jun 10, 2024
@renatho renatho marked this pull request as ready for review June 10, 2024 20:24
@renatho renatho requested a review from a team June 10, 2024 20:24
@github-actions GitHub Actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@Imran92
Copy link
Contributor

Imran92 commented Jun 12, 2024

Just closing and reopening to fix the milestone validation check

@Imran92 Imran92 closed this Jun 12, 2024
@Imran92 Imran92 reopened this Jun 12, 2024
@github-actions GitHub Actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@github-actions GitHub Actions
Copy link

Test the previous changes of this PR with WordPress Playground.

Imran92
Imran92 previously approved these changes Jun 12, 2024
View reviewed changes
Copy link
Contributor

@Imran92 Imran92 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good and works as expected. Just added one comment about the test name convention. Also Is the failing test relevant to this PR?

tests/unit-tests/rest-api/test-class-sensei-rest-api-lesson-quiz-controller.php Outdated
/**
* Tests posting multiple choice question with empty string as an answer.
*/
public function testPostMultipleChoiceQuestion_WhenAnswersContainsEmptyString_AnswerIsRemoved() {
Copy link
Contributor

Choose a reason for hiding this comment

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

When we are calling the send_post_request method, it's probably calling the save_quiz method through the API. So the test is actually for the save_quiz method. As per the naming convention doc here - p6rkRX-35r-p2, the name of the test should probably be testSaveQuiz. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review, Imran!

Fixed the test format here: 126cb95

I also added the "Arrange", "Act" and "Assert" comments that I had forgotten.

@renatho
Copy link
Contributor Author

renatho commented Jun 12, 2024

Also Is the failing test relevant to this PR?

It's not. Just to confirm, I created another PR and it had the same issue: https://github.com/Automattic/sensei/actions/runs/9486813834/job/26142021297?pr=7617

@github-actions GitHub Actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@renatho
Copy link
Contributor Author

renatho commented Jun 12, 2024

Since the last changes were only related to tests format, I'm merging this for the coming release. @Imran92, if you see anything else in my change, let me know that I can open a new PR. 😉

@renatho renatho merged commit e0f34bf into trunk Jun 12, 2024
22 of 23 checks passed
@renatho renatho deleted the fix/quiz-questions-with-falsy-answers branch June 12, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Imran92 Imran92 Imran92 left review comments

Assignees

@renatho renatho

Labels
None yet
Projects
None yet
Milestone
4.24.1
Development

Successfully merging this pull request may close these issues.

Zero (0) result disappearing in Multiple Choice
2 participants
@renatho @Imran92