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

Prevent sending welcome email immediately on enrole in case their access starts in the future #7615

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from
Open

Prevent sending welcome email immediately on enrole in case their access starts in the future #7615

wants to merge 11 commits into from

Conversation

Imran92
Copy link
Contributor

@Imran92 Imran92 commented Jun 11, 2024 •
edited
Loading

Resolves https://github.com/Automattic/sensei-pro/issues/2572
(Partially)

Proposed Changes

  1. We have switched from hook sensei_user_course_start to sensei_course_enrolment_status_changed because sensei_user_course_start hook is fired before the access start date is set. So it's not possible at that point to prevent or allow this email based on access. sensei_course_enrolment_status_changed is fired after everything is in place.

  2. We have improved our base email generator class. Now it allows you to listen to multiple hooks. Previously you could only attach to one.

  3. Updated the sensei_send_emails to contain the placeholders as well. This will allow us to add listener hook functions that take decisions based on relevant values.

Testing Instructions

  1. Disable your local sensei-pro
  2. Create two Courses
  3. Enroll as a student
  4. Make sure your receive your welcome email immediately
  5. Go to Sensei LMS -> Emails
  6. Disable the welcome email in the student tab
  7. Now enroll in another course
  8. Make sure that you haven't received a welcome email this time
  9. Enable Sensei Pro, then follow the instructions here https://github.com/Automattic/sensei-pro/pull/2586

New/Updated Hooks

  • sensei_send_emails - Added placeholders param.

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 added 11 commits June 3, 2024 05:32
@Imran92
Add extra replacements param in email hook
5cb38d7 
It will help the function listening to the hook to make changes depending on the params
@Imran92
Send replacement param in email hook
3f27646
@Imran92
Allow email to be called on multiple action
c3a0fcf
@Imran92
Add action to send email on course access start
27fc934
@Imran92
Add test to make sure both event gets registered
847baa4
@Imran92
Update hook name
5906d7e
@Imran92
Use different hook to access the start date
68e34e4
@Imran92
Update test to use new hook
35e594f 
Because the previous hook called this function before the access start date was even set
@Imran92
Make is_enrolled optiona
599c4b8
@Imran92
Change the hook
7a8ea95 
The previous hook got called 'before' the access dates were set. So it was not possible to check the access date and decide if the email should be sent or not. This hook is called after the date is set. So it's possible to send the email conditionally by checking the access start date
@Imran92
Merge branch 'trunk' into fix/prevent-welcome-email-for-future-access
818e048
@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 added this to the 4.24.1 milestone Jun 11, 2024
@Imran92 Imran92 added the Hooks This change adds or modifies one or more hooks. label Jun 11, 2024
@Imran92 Imran92 closed this Jun 11, 2024
@Imran92 Imran92 reopened this Jun 11, 2024
@Imran92 Imran92 self-assigned this Jun 11, 2024
@Imran92 Imran92 requested a review from a team June 11, 2024 01:13
@Imran92 Imran92 marked this pull request as ready for review June 11, 2024 01:13
@github-actions GitHub Actions
Copy link

Test the previous changes of this PR with WordPress Playground.

@renatho renatho self-requested a review June 11, 2024 19:58
renatho
renatho approved these changes Jun 11, 2024
View reviewed changes
Copy link
Contributor

@renatho renatho 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 and works well to me!

Just about the Sensei Pro PR, maybe is better to take a look before merging this, just in case you need to tweak something related to that in this PR. 😉

@donnapep donnapep modified the milestones: 4.24.1, 4.24.2 Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@renatho renatho renatho approved these changes

Assignees

@Imran92 Imran92

Labels
Hooks This change adds or modifies one or more hooks. No Changelog
Projects
None yet
Milestone
4.24.2
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Imran92 @renatho @donnapep