Fix draft lessons getting published when Course is updated #7582

Imran92 · 2024-04-05T17:56:44Z

Resolves #7555

Proposed Changes

Even though the lessons were supposed to get published only at the time of the Course getting published, they were getting published on all updates of course.

If we look at the code, it's attached to the publish_course hook, so it should get called ONLY when the Course is getting published, right?

Well, no! If we look at the documentation of the hook, in the bottom, it says -

Please note: When this action is hooked using a particular post status (like ‘publish’, as publish_{$post->post_type}), it will fire both when a post is first transitioned to that status from something else, as well as upon subsequent post updates (old and new status are both the same).

So it was getting called everytime for a published Course, and lessons were getting published.

But luckily, there's an old_status param, which shows which status this post is transitioning from. So it should be just as simple as just checking if the Course is getting Published from an old status of !== 'publish' right?

Well, no! To understand the reason, you need to know how we save our Course structure from GB editor. When you create a Course, add a bunch of lessons and click on Publish, or when you edit the Course outline block, and click on Update in an existing course, first a call is initiated by GB to save the Course markup (it doesn't save the lessons, modules, or the course structure), parallelly, another call is initiated to save the Metabox values. Once these two are completed, Sensei sends an API call with the payload of the Course structure in JSON, this is when the Lessons get created/updated. After that, an (or multiple) update calls are made to sync the frontend and backend Course structure.

So, even if we try to publish the lessons at the time of the transition of the Course, it won't work, because, the lessons do not even exist at the time of the Course getting published! (The first GB call).

So we can do it in the later call right? Yap, but how do you differentiate it from a normal Update call? To do that, we've set a flag that's set at the time of the transition, and gets cleared after the subsequent call. Is that all?

Well, no! because if we attach the hook inside the is_admin() check, it will not get attached for the first GB call, so inside the hook, we won't even know that it's transitioning to Publish. Why does a GB call from the editor made inside Admin panel is getting is_admin() to return false you ask? I don't know, haven't looked into it yet (planning to do later after publishing this PR).

A couple of things I've noticed:
If lessons are already in draft (not unsaved, draft) in a draft course, then clicking on publish doesn't immediately update the UI of the lessons remove the draft tag from below them. But it's not newly introduced, it was existing, I haven't fixed it here.
And it's technically possible to still publish a lesson in a subsequent update if you follow a special sequence (but you kinda have to intentionally try it). So I haven't covered that case. For general testing and use case flows, it should be enough.

Testing Instructions

Create a Course
Add lessons
Publish with the publish lessons Checkbox active
Lessons should get published
Add another lesson in that Course
Update the Course
The new Lesson should not get published
Try in a similar manner with existing Courses

Pre-Merge Checklist

Because for the actual publish call sent by default by Gutenberg, is_admin is false. So it will never catch the actual publish call

github-actions · 2024-04-05T18:04:22Z

Test the previous changes of this PR with WordPress Playground.

codecov · 2024-04-05T18:08:13Z

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 51.77%. Comparing base (7742f33) to head (2ac96e8).
Report is 21 commits behind head on trunk.

Additional details and impacted files

@@            Coverage Diff             @@
##              trunk    #7582    +/-   ##
==========================================
  Coverage     51.76%   51.77%            
- Complexity    11323    11401    +78     
==========================================
  Files           641      650     +9     
  Lines         48185    48549   +364     
  Branches        470      470            
==========================================
+ Hits          24943    25135   +192     
- Misses        22861    23033   +172     
  Partials        381      381

Files	Coverage Δ
...es/admin/class-sensei-course-pre-publish-panel.php	`71.05% <93.33%> (+19.20%)`	⬆️
includes/class-sensei.php	`24.89% <0.00%> (ø)`

... and 42 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 0132d5c...2ac96e8. Read the comment docs.

donnapep · 2024-04-08T13:49:03Z

Thanks for the write-up!

So, even if we try to publish the lessons at the time of the transition of the Course, it won't work, because, the lessons do not even exist at the time of the Course getting published! (The first GB call).

I don't get why it worked in the original PR then, if the lessons weren't even created at that point?

donnapep

I tested this, but step 4 (Lessons should get published) didn't work. My lessons are still in Draft. On trunk branch they are published.

tests/unit-tests/admin/test-class-sensei-course-pre-publish-panel.php

donnapep · 2024-04-08T14:27:05Z

includes/admin/class-sensei-course-pre-publish-panel.php

+
+		if ( $is_main_publish_call ) {
+			// This is the first call made, it's not the structure saving call, so the added/updated lessons are not yet saved at this point.
+			// So we set the flag to publish lessons on the next call, which is made after the structure is saved.


I find this comment a bit confusing. You reference a "next call" here, saying that this next call will be made after the structure is saved. But in the previous sentence, you said that this call is also not the structure saving call, so it's unclear from these comments when the call to save the structure actually happens.

The saving process is a bit complex :p So I was struggling a bit a capture it in small comments. Basically, when we click on Publish, GB makes a call with the markup. We are referring to it as the Publish call, this is the first call.

If you check the code here -

sensei/assets/shared/structure/structure-store.js

Line 253 in 0132d5c

editor.isSavingPost() && ! editor.isAutosavingPost();

, you'll see that we wait for the Save/Publish call to complete before sending the call to save the structure (This is the Structure Saving Call) -

sensei/assets/shared/structure/structure-store.js

Line 274 in 0132d5c

dispatch( storeName ).startPostSave();

After the structure is also saved, we trigger another GB call (not a structure saving call, a same call as the first Publish call, but the difference is by this time, the Course is already published)

sensei/assets/shared/structure/structure-store.js

Line 158 in 0132d5c

yield* actions.savePost();

And this last call is the one I am referring to in the comments

I understand the logic, but I still think the comment needs to be improved a bit to eliminate confusion for the next person. 🙂 Perhaps you could add a numbered list of the calls that are made somewhere in this function to "set the stage" so to speak, indicating which ones are always called and which ones may be called? I think it's more important for the comment to be clear than to be succinct.

Thanks @donnapep ! I've rewrote all the comments here 2ac96e8 . Please LMK if you think the new structure is okay. Initally I was trying to use fewer words as I've not seen this many comments in the repo before for a function, so I was afraid if I was overdoing it :p Tried to make it clearer now

includes/admin/class-sensei-course-pre-publish-panel.php

Imran92 · 2024-04-08T16:21:11Z

I don't get why it worked in the original PR then, if the lessons weren't even created at that point?

It's only when you are adding new unsaved lessons and clicking on the Publish button.

But let's say you have created a Course, added a bunch of Lessons and clicked on "Save Draft". Later you come back to this course, edit something or maybe not, and then click on Publish, in that case the lessons will already be created, because they got created the last time you clicked on "Save Draft"

github-actions · 2024-04-08T16:21:41Z

Test the previous changes of this PR with WordPress Playground.

Imran92 · 2024-04-08T16:26:36Z

I tested this, but step 4 (Lessons should get published) didn't work. My lessons are still in Draft. On trunk branch they are published.

Thanks for finding this case! Actually it's due to another behavior I've noticed now. If you add the lessons fast after creating the Course, they got added as "Draft", but if you do it slower, they get added as "Unsaved". The issue was when in unsaved state, metabox saving call (auto generated by GB) also triggered the publish_course hook before the structure got saved, causing our flag to not work. I've fixed it here 8eda32e, so now it should work for all cases

Fast

Screen.Recording.2024-04-08.at.10.03.47.PM.mov

Slow

Screen.Recording.2024-04-08.at.10.04.56.PM.mov

github-actions · 2024-04-08T17:15:57Z

Test the previous changes of this PR with WordPress Playground.

donnapep

Looks good! I just left another small note about comments here and here to make it easier for future devs as this flow is a bit complex.

github-actions · 2024-04-10T18:53:35Z

Test the previous changes of this PR with WordPress Playground.

Imran92 · 2024-04-10T19:21:10Z

Looks good! I just left another small note about comments #7582 (comment) and #7582 (comment) to make it easier for future devs as this flow is a bit complex.

Thanks @donnapep ! I've updated the comments here 2ac96e8 . Please LMK if it looks better now.

Thank you!

Imran92 added 11 commits April 5, 2024 19:45

Move outsite is admin

da19d8a

Because for the actual publish call sent by default by Gutenberg, is_admin is false. So it will never catch the actual publish call

Publish lessons only on course publish, not update

d5fde01

Explain why we return when we detect it's just update call

ab99b76

Explain why we reset the flag

e42968d

Add changelog

18e51d0

Fix old tests

25997be

Test that we don't publish draft lessons during course update

75cf1ca

Test that publishing course not publishing lesson when meta false

397a263

Test that flag is set during publish

0cbd25b

Test that lesson get published in subsequent immediate call

bd304e9

Add test that for more subsequent calls, it doesn't publish

45c7eb2

Imran92 added this to the 4.23.1 milestone Apr 5, 2024

Imran92 requested a review from a team April 5, 2024 17:56

Imran92 self-assigned this Apr 5, 2024

donnapep reviewed Apr 8, 2024

View reviewed changes

Imran92 added 2 commits April 8, 2024 22:15

Add check for metabox save call

8eda32e

Add test for metabox save call

cba86c2

Add comments to tests

137a105

Imran92 requested a review from donnapep April 8, 2024 17:11

donnapep previously approved these changes Apr 9, 2024

View reviewed changes

Add comments explaining course saving process

2ac96e8

Imran92 dismissed donnapep’s stale review via 2ac96e8 April 10, 2024 18:44

Imran92 requested a review from donnapep April 10, 2024 19:21

donnapep approved these changes Apr 11, 2024

View reviewed changes

donnapep merged commit ae239a2 into trunk Apr 11, 2024
25 checks passed

donnapep deleted the fix/publishing-lessons-in-course-updates branch April 11, 2024 12:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix draft lessons getting published when Course is updated #7582

Fix draft lessons getting published when Course is updated #7582

Imran92 commented Apr 5, 2024

github-actions bot commented Apr 5, 2024

codecov bot commented Apr 5, 2024 •

edited

donnapep commented Apr 8, 2024

donnapep left a comment

donnapep Apr 8, 2024

Imran92 Apr 8, 2024 •

edited

donnapep Apr 9, 2024 •

edited

Imran92 Apr 10, 2024

Imran92 commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

Imran92 commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

donnapep left a comment •

edited

github-actions bot commented Apr 10, 2024

Imran92 commented Apr 10, 2024

Fix draft lessons getting published when Course is updated #7582

Fix draft lessons getting published when Course is updated #7582

Conversation

Imran92 commented Apr 5, 2024

Proposed Changes

Testing Instructions

Pre-Merge Checklist

github-actions bot commented Apr 5, 2024

codecov bot commented Apr 5, 2024 • edited

Codecov Report

donnapep commented Apr 8, 2024

donnapep left a comment

Choose a reason for hiding this comment

donnapep Apr 8, 2024

Choose a reason for hiding this comment

Imran92 Apr 8, 2024 • edited

Choose a reason for hiding this comment

donnapep Apr 9, 2024 • edited

Choose a reason for hiding this comment

Imran92 Apr 10, 2024

Choose a reason for hiding this comment

Imran92 commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

Imran92 commented Apr 8, 2024

Fast

Slow

github-actions bot commented Apr 8, 2024

donnapep left a comment • edited

Choose a reason for hiding this comment

github-actions bot commented Apr 10, 2024

Imran92 commented Apr 10, 2024

codecov bot commented Apr 5, 2024 •

edited

Imran92 Apr 8, 2024 •

edited

donnapep Apr 9, 2024 •

edited

donnapep left a comment •

edited