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

Hide comments when lesson has not dripped #173

Merged
merged 1 commit into from Nov 11, 2019

Conversation

@gkaragia
Copy link
Contributor

gkaragia commented Oct 25, 2019

Fixes #139

Overview

This change fixes the issue that lesson comments where visible although the lesson was not accessible due to content drip. It relies on the filter that was added with this PR.

Testing

  1. In Sensei settings, make sure that General->Access Permissions and Lessons->Allow Comments for Lessons are enabled.
  2. Create a lesson and with a user add a comment to it.
  3. By using content drip, make the lesson inaccessible by setting a future date that it will be accessible.
  4. With another user subscribe to the course.
  5. Open the lesson page and observe that the comments are not displayed.
  6. Update the lesson again to make it accessible.
  7. Check again and see that now the comments are displayed.

Notes

This change has a dependency on this change in Sensei core. Although there will be no errors if one of the two is merged without the other, we need both for the functionality to work.

@gkaragia gkaragia requested review from jom, alexsanford and donnapep Oct 25, 2019
Copy link
Contributor

alexsanford left a comment

Instead of adding a filter to Sensei, what do you think of just removing the action that outputs the comments? I.e.

remove_action( 'sensei_pagination', [ 'Sensei_Lesson', 'output_comments' ], 90 );

(See https://github.com/Automattic/sensei/blob/83fd3610e6b7483a5b1a665179e5fe511e32709e/includes/hooks/template.php#L296)

Of course we would need to wrap it in the logic for whether the comments should be displayed.

The benefit of doing this is that we wouldn't have to add another filter, and we wouldn't be dependent on the Sensei release (with the new filter) in order for this to work.

@gkaragia

This comment has been minimized.

Copy link
Contributor Author

gkaragia commented Oct 29, 2019

I think that in general the solution with the new hook is a bit cleaner since with the new hook all the functionality is going to be in a single place, in output_comments method.

However adding a hook increases complexity and introduces this dependency so I think that it's worth to remove the existing action instead.

@alexsanford

This comment has been minimized.

Copy link
Contributor

alexsanford commented Oct 29, 2019

However adding a hook increases complexity and introduces this dependency

I agree, and it also means that there are two ways to do the same thing (i.e. use the filter or remove the action). Either one is a common practice in WordPress-land, so having the two different ways could cause confusion.

Maybe 🤷‍♂

@gkaragia gkaragia force-pushed the fix/hide-comments-when-lesson-blocked branch from 2692c3c to fd67fe1 Oct 31, 2019
@gkaragia gkaragia requested a review from alexsanford Oct 31, 2019
Copy link
Contributor

donnapep left a comment

Couple of comments, but tests well!

includes/class-scd-ext-lesson-frontend.php Outdated Show resolved Hide resolved
includes/class-scd-ext-lesson-frontend.php Outdated Show resolved Hide resolved
@donnapep donnapep added this to the 2.0.1 milestone Oct 31, 2019
@gkaragia gkaragia force-pushed the fix/hide-comments-when-lesson-blocked branch from fd67fe1 to 32a02af Oct 31, 2019
@gkaragia gkaragia requested a review from donnapep Oct 31, 2019
@jom
jom approved these changes Nov 8, 2019
Copy link
Contributor

jom left a comment

Looks good, works well! 👍

@gkaragia gkaragia merged commit d8d7f58 into master Nov 11, 2019
@gkaragia gkaragia deleted the fix/hide-comments-when-lesson-blocked branch Nov 11, 2019
@donnapep donnapep changed the title Add filter to hide comments when lesson is blocked Hide comments when lesson is not available yet Nov 29, 2019
@donnapep donnapep changed the title Hide comments when lesson is not available yet Hide comments when lesson has not dripped Nov 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.