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

Update course notices to have a CTA for course editors #7403

Merged
merged 18 commits into from
Jan 5, 2024

Conversation

renatho
Copy link
Contributor

@renatho renatho commented Dec 22, 2023

Resolves #7374

Proposed Changes

  • Update the course notices, adding a CTA for users that can manage the course.
  • I took the opportunity to move the notices logic from the block rendering to a hook because it wasn't using a good practice. More context: pdxWSz-1aV-p2
  • It fixes an issue with redirect when taking a course while lessons are in draft.
  • Fix an issue with the message "Cannot register for an unpublished course.". It wasn't working because it was redirecting the user to the first lesson.

Nice-to-have: It would be nice to left-align the notice text. This isn't a mandatory requirement for this issue though. I imagine it will affect all notices (which is probably a good thing).

I was afraid to break other notices, so I didn't do it as part of this PR. Maybe we could have another issue to think it better, reorganizing the HTML and styles of this notice, and maybe having a better design for it?

Testing Instructions

Published course containing only draft lessons

  1. Create and publish a course containing at least one draft lesson.
  2. Access it in the frontend as a course editor (teacher/admin).
  3. Check that you see a message with a link to publish the lessons.
  4. Access it as a student, and make sure you see a message that the course doesn't have published lessons, and you don't see any CTA.

Published course without any lesson

  1. Create and publish a course without any lesson.
  2. Access it in the frontend as a course editor (teacher/admin). Check that you see a message that there are no lessons, and the link to publish the lessons.
  3. Access it as a student, and make sure you see the message but no link to publish lessons.

Draft course

  1. Create a draft course.
  2. Access it in the frontend as a course editor (teacher/admin).
  3. Try to take this course and make sure you see a notice that you can't take the course and a button to publish it.

Redirect issue

  1. Create and publish a course containing only draft lessons.
  2. Access it in the frontend as a course editor (teacher/admin).
  3. Try to take the course, and make sure you are properly redirected to the first draft lesson.

Notice that we don't have a message when the lessons are published, but the course is in draft. Should we add a message for this case?

Screenshots

Student visiting a course with no lessons (or all draft):

Screenshot 2023-12-22 at 18 07 20

Admin visiting a course with at least one draft lesson:

Screenshot 2023-12-22 at 18 59 46

Admin visiting a course without lessons:

Screenshot 2023-12-22 at 17 12 43

Admin trying to take a draft course:

Screenshot 2023-12-22 at 18 06 04

Deprecated Code

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
  • 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

It should continue in the course to display a notice that can't enroll when course is draft.
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (c4960ff) 50.95% compared to head (e1885dd) 51.03%.
Report is 10 commits behind head on trunk.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##              trunk    #7403      +/-   ##
============================================
+ Coverage     50.95%   51.03%   +0.08%     
- Complexity    11156    11165       +9     
============================================
  Files           613      613              
  Lines         47073    47137      +64     
  Branches        404      405       +1     
============================================
+ Hits          23986    24058      +72     
+ Misses        22760    22752       -8     
  Partials        327      327              
Files Coverage Δ
assets/shared/query-string-router/index.js 90.47% <100.00%> (+3.80%) ⬆️
...ludes/blocks/class-sensei-course-outline-block.php 93.75% <100.00%> (+15.17%) ⬆️
...locks/class-sensei-course-outline-course-block.php 83.87% <ø> (+3.31%) ⬆️
includes/class-sensei-admin.php 19.02% <0.00%> (-0.02%) ⬇️
includes/class-sensei-frontend.php 17.43% <0.00%> (ø)
includes/class-sensei-course.php 39.05% <0.00%> (-0.05%) ⬇️

... 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 d38577f...e1885dd. Read the comment docs.

@renatho renatho self-assigned this Dec 22, 2023
@renatho renatho added this to the 4.20.1 milestone Dec 22, 2023
@renatho renatho changed the title Update course notices to have a CTA for the admin Update course notices to have a CTA for course editors Dec 22, 2023
@renatho renatho requested a review from a team December 22, 2023 23:18
@renatho renatho marked this pull request as ready for review December 22, 2023 23:19
@renatho renatho marked this pull request as draft December 23, 2023 09:55
@renatho
Copy link
Contributor Author

renatho commented Dec 23, 2023

I just thought that I didn't consider the legacy courses (with no course outline).
It's probably duplicating the messages. I think I need to add a check here if the course outline is in the post content.

@renatho
Copy link
Contributor Author

renatho commented Dec 27, 2023

I just thought that I didn't consider the legacy courses (with no course outline).
It's probably duplicating the messages. I think I need to add a check here if the course outline is in the post content.

I just checked that the legacy course didn't have the respective notices. So I kept it even without the Course Outline.

Screenshot 2023-12-27 at 14 49 01 Screenshot 2023-12-27 at 14 46 51 Screenshot 2023-12-27 at 14 48 06

Notice that the position of the notices are a little weird because of this issue: #6173

@renatho renatho marked this pull request as ready for review December 27, 2023 17:56
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

  • It looks like the "your lessons" link is currently going to the course editor, but as per the issue:

clicking on "your lessons" takes the user to the Lessons page (wp-admin/edit.php?post_type=lesson) in WP admin, filtered by the current course and by Draft status.

  • For a course with no lessons, I see this message as an admin:

Screenshot 2024-01-04 at 10 54 59 AM

Since there are no lessons in the course, the sentences contradict each other. Could we use this text instead?

There are no published lessons in this course yet. Add some now.

where Add some now. is a link that goes to the course editor. (It would be even better if the link went to the Lesson editor with the Course dropdown pre-populated to the appropriate course, but I guess that will be tricky so linking to the course editor is fine. 🙂)

Screenshot 2024-01-04 at 11 01 24 AM

changelog/fix-redirect-on-enrollment-with-draft-lessons Outdated Show resolved Hide resolved
changelog/fix-cannot-register-unpublished-course-notice Outdated Show resolved Hide resolved
includes/blocks/class-sensei-course-outline-block.php Outdated Show resolved Hide resolved
renatho and others added 2 commits January 4, 2024 13:29
Co-authored-by: Donna Peplinskie <donnapep@gmail.com>
Co-authored-by: Donna Peplinskie <donnapep@gmail.com>
@renatho
Copy link
Contributor Author

renatho commented Jan 4, 2024

Thank you for the review, @donnapep!

Messages updated here: b85b748

@renatho renatho requested a review from donnapep January 4, 2024 18:00
Copy link
Collaborator

@donnapep donnapep left a comment

Choose a reason for hiding this comment

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

I made a tweak in 8390d01 after changing my mind. 😃 It it looks OK, we're 👍🏻 to merge.

@renatho renatho merged commit df9beba into trunk Jan 5, 2024
24 checks passed
@renatho renatho deleted the change/course-notices branch January 5, 2024 17:16
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.

Add a CTA to the unpublished lessons notice
2 participants