Always allow access to a lesson if the lesson preview is enabled #7655
Conversation
|
Test the previous changes of this PR with WordPress Playground. |
| @@ -173,7 +172,9 @@ function sensei_can_user_view_lesson( $lesson_id = null, $user_id = null ) { | |||
| * @param {int} $user_id User ID. | |||
| * @return {bool} Filtered access. | |||
| */ | |||
| return apply_filters( 'sensei_can_user_view_lesson', $can_user_view_lesson, $lesson_id, $user_id ); | |||
There was a problem hiding this comment.
I'm a little concerned with this approach because this was filtering the final state of the sensei_can_user_view_lesson.
What if users have other customizations trusting on this filter with the is_preview_lesson?
Even on Sensei Pro, I see that we have a few more cases using this filter. Maybe they could break in some special cases because of this change?
Another solution I could think of is adding another argument to this filter with the values of the things were checked ($statuses, for example?), so when using the filter we could make different decisions based on the values. Like checking on Sensei Pro for this specific scenario if $statuses['is_preview'] is true.
If you agree, I'm just not sure how we could do it in a backward compatibility way. I think if we call a filter with 4 arguments and if has 3, it gives an error, right? Maybe we would need a version check?
There was a problem hiding this comment.
Yes, it makes sense for me. And I'm ready to accept your suggestion as an adequate alternative.
But I want you to consider the following and express your opinion.
I was thinking about that, considering different approaches, including passing an additional argument.
My final thought was that the Preview setting is a fundamental one, and we must respect it. But it was easy to overlook it, even though it was possible to get it in the hooks (not directly through arguments).
Exactly that happened in the original issue (the case you mentioned about Sensei Pro).
This is why I came to a solution where I force the effect of the preview setting.
Discussing your suggestion, it sounds viable. Adding the fourth argument sounds safe, but will require to update all plugins that use the hook. The main ones are under our control.
At the same time, it feels there is no backward incompatibility. The last argument with additional statuses could be considered as an optional one. It doesn't require immediate changes if they don't care about those statuses. Currently, we'll need to do an update only in one place in Sensei Pro.
The downside, from my perspective, is that we can't enforce the Preview option working. But it could be considered as an advantage because of flexibility.
There was a problem hiding this comment.
My final thought was that the Preview setting is a fundamental one, and we must respect it.
I think it makes sense for the course expiration, but could not make sense for other cases. Let's say for example that I only want to allow access to the content of my courses for a specific country or for bots. In these cases we'd probably also want block access even for preview lessons.
My final thought was that the Preview setting is a fundamental one, and we must respect it. But it was easy to overlook it, even though it was possible to get it in the hooks (not directly through arguments).
Hmm... 🤔
Thinking more about this, other thoughts came to my mind.
We have one more check that we could consider "fundamental", which is the access for admins. We probably don't want admin users or course owners losing access, right?
What if we have 2 hooks? Keeping the original one with all the checks, and add one before for the non-fundamental checks?
Something like this (Ignore details like names, arguments, code structure. It's just to illustrate what I mean):
$non_fundamental_checks = apply_filters( 'non_fundamental_checks', ! sensei_is_login_required() || ( $user_can_view_course_content && $pre_requisite_complete, ! sensei_is_login_required(), $user_can_view_course_content, $pre_requisite_complete );
$fundamental_checks = apply_filters( 'fundamental_checks', $is_preview || $sensei_all_access, $is_preview, $sensei_all_access );
$can_user_view_lesson = $non_fundamental_checks || $fundamental_checks;
return apply_filters( 'sensei_can_user_view_lesson', $can_user_view_lesson, $lesson_id, $user_id );The only problem I'd see with this last idea I shared is if the "fundamental" concept is valid for any scenario. 🤔 Maybe the other one with an argument would be more open to any type of scenario and easier to understand. WDYT?
There was a problem hiding this comment.
Thanks, @renatho.
Based on the discussion, I created two other PRs:
- Pass checks argument to sensei_can_user_view_lesson filter #7657
- https://github.com/Automattic/sensei-pro/pull/2597
I applied your first suggestion there.
I'm closing this PR.
Resolves #7630
In the original issue, the Preview setting didn't work if the user's access expired.
Here, we prioritize the Preview setting so that it will have higher priority over other settings.
Proposed Changes
Testing Instructions
Pre-Merge Checklist