Don't send Course Completion email twice (course already complete)

[merkushin]

Resolves #7404

Proposed Changes

• Add additional argument to sensei_course_status_updated hook, providing previous status.
• Don't send Course Completion email if the previous status is "complete".

Testing Instructions

1. Create a course with at least two lessons.
2. As a student, complete both lessons.
3. When you appear on the Course Complete page, use browser navigation to go back to the first lesson in the course, then use browser navigation to go forward to the end.
4. It is expected to get a single Course Completion email.

New/Updated Hooks

• sensei_course_status_updated — fourth param added: $previous_status — status before updating to the new one.

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

[merkushin]

Should we check other generators and maybe add a similar "previous" status there?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b612201) 50.94% compared to head (c4960ff) 50.95%.
Report is 4 commits behind head on trunk.

Additional details and impacted files

@@            Coverage Diff            @@
##              trunk    #7405   +/-   ##
=========================================
  Coverage     50.94%   50.95%           
- Complexity    11153    11156    +3     
=========================================
  Files           613      613           
  Lines         47067    47073    +6     
  Branches        404      404           
=========================================
+ Hits          23980    23986    +6     
  Misses        22760    22760           
  Partials        327      327           

Files	Coverage Δ
includes/class-sensei-utils.php	52.78% <100.00%> (+0.07%)	⬆️
...udes/internal/emails/class-email-page-template.php	75.00% <100.00%> (ø)
...ernal/emails/generators/class-course-completed.php	100.00% <100.00%> (ø)

---

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 33867f0...c4960ff. Read the comment docs.

[renatho]

Hi @merkushin!

The code looks good to me. I just added 2 small comments about the docs.

I tried to complete the course twice by navigating back, but I couldn't. Maybe I'm doing something in a different way?

Screen.Recording.2023-12-27.at.11.26.05.mov

I recorded it as an admin user, but after that I also tested as a student, and I had the same behavior.

[Imran92]

As there is a change in the hook, probably we should add the hooks label as well

[merkushin]

@renatho Oh no, you're right. I confused two different emails (one for the teacher and another one for the student), thinking this is the same one when I go back and then forward with the browser navigation 🤦 But the solution I tested on a different website where I have different emails ("physically") for the teacher and for the student.
I'm going to try to reproduce it without Learning Mode. Maybe there the submission is triggered...

Update: tried with and without Learning Mode, with the Course and Astra themes, in Chrome and in Edge. And I can't reproduce the same behavior as the user described. Worth noticing, they said they can't reliably reproduce it in all browsers and they suppose it might depend on browser settings.

[renatho]

Worth noticing, they said they can't reliably reproduce it in all browsers and they suppose it might depend on browser settings.

I see! So I'm already approving it, in case you think it's the best approach.

But I'd like to propose another thing to think about and see what you think. In this case, wouldn't be better to completely skip the event earlier? So maybe we could take the opportunity to avoid other issues like this in the future. Maybe we could also log an error? I'd say display an error message to the user but I imagine it would be something more complicated, right?

Anyway, if you think your current approach is better after your exploration, feel free to merge it.

[merkushin]

Thanks @renatho!

wouldn't be better to completely skip the event earlier?

I spent some time reflecting on it. I think we shouldn't stop firing the event. The hook was added many years ago. Changing the behavior sounds risky. And I can't say securely that's worth doing from the app logic viewpoint: I can imagine useful use-cases.

So maybe we could take the opportunity to avoid other issues like this in the future. Maybe we could also log an error? I'd say display an error message to the user but I imagine it would be something more complicated, right?

Here, I think, the important thing is that it is not an error. What I mean is that the end user (a student) shouldn't care about what happens under the hood. The app decides if a message should be sent.
For a developer, we can log the event only when we checked the condition. But that means we should log each normal case of usage. Which means the logs will be flooded with messages that don't make sense.

As I can get the problem, it happens because of some client-side caching. Usually, to avoid such cases, we redirect a user after every POST-request. The same we do here. (Worth noticing that the user doesn't experience issues with Lesson Completion emails!)

To sum up, I think in this PR we improve the logic a bit by checking the previous status, but not harm anyone with unnecessary logs or error messages.