sales page url feedback

[thomasplevy]

@thomasplevy I think we need a different course's sales_page_url. Looking at:

so the lifterlms core the sales_page_url is not what the specs are describing right now. The specs are calling sales_page_url what for the corse is sales_page_content_url https://github.com/gocodebox/lifterlms/blob/3.33.2/includes/models/model.llms.course.php#L276

I think we can change the specs so that sales_page_url is a read-only param which equals to LLMS_Course->get_sales_page_url(), while sales_page_type, sales_page_page_id, sales_page_content_url (the former sales_page_url) are available in both edit and view contexts.

Though maybe we can also consider to be more restrictive and make the latter 3 available in edit context only. I mean, if you're a viewer should you really know how something has been built? I guess no, you shouldn't...

what do you think?

Originally posted by @eri-trabiccolo in https://github.com/gocodebox/lifterlms-rest/issues/6#issuecomment-509326272

[thomasplevy]

@eri-trabiccolo I understand the issue here but I don't know if the rest api should be returning parameters which preform "business logic" like the function in question.

If you think it's confusing to change the parameter name (which I shortened because having "content" as part of the parameter name doesn't feel essential to me) is the issue I'm open to changing it.

I think the method get_sales_page_url() preforms logic which the REST API consumer should be preforming on their own. The REST (I think) should be returning data. The consumer preforms the logic.

Maybe I'm wrong though...

[eri-trabiccolo]

@eri-trabiccolo I understand the issue here but I don't know if the rest api should be returning parameters which preform "business logic" like the function in question.

mmm

If you think it's confusing to change the parameter name (which I shortened because having "content" as part of the parameter name doesn't feel essential to me) is the issue I'm open to changing it.

Nope that's fine for me. :D And more than the logic there I was thinking about that filter...

I think the method get_sales_page_url() preforms logic which the REST API consumer should be preforming on their own. The REST (I think) should be returning data. The consumer preforms the logic.

Maybe I'm wrong though...

Theoretically I'm with you but... I guess it's because WP always messes up concepts and my thoughts too :D Of course I'm referring to the "rendered" content thing. That's not the the data, and probably not even the "raw" is the data: since there are shortcodes/blocks and tons of plugins that can filter (alter) the content, the "rendered" prop is kinda needed; but why don't expose the "raw" content in view mode then?!

In any case WP gives the ability to access (with certain caps) both the "raw" and the "rendered" version of some props, while we are not allowing the consumer to access to the result of the filters applied to this hook 'llms_course_get_sales_page_url', this means that, potentially if I build a mobile app using the REST API I might never be able to reproduce the same behaviour the WP website shows. This is what bugged me (conceptually, rather than in this specific case).

Having said that, we can always decide this is the way we want it, and/or decide case by case what to process and what not, and in this very case it's a no no.

[eri-trabiccolo]

In the October bug scrub we discussed a little more in depth on how to handle the llms posts restrictions and we decided to do as following: 1) use the llms core api (llms_page_resctricted($id)) to retrieve the restriction status of the post (force the wp_query fields as needed by the llms core api, e.g. is_singular); 2) add a new ro sub-property to the resource's content property (and possibly to the others affected by the restriction), called restricted, that stores the restriction state so to give more information to the consumer about why the related property looks like that; 3) a restriction that implies a redirect would result in an empty content property (at least).

[eri-trabiccolo]

@thomasplevy I've spent some time looking the "redirect" matter: what rendered content (empty?!) display when a post is restricted + the llms core forces a redirection.

All the redirection logic, as far as I can see, is here: https://github.com/gocodebox/lifterlms/blob/3.36.2/includes/class.llms.template.loader.php#L10 mostly: https://github.com/gocodebox/lifterlms/blob/3.36.2/includes/class.llms.template.loader.php#L411

Now for courses and memberships we can just look at the sales page options(https://github.com/gocodebox/lifterlms/blob/3.36.2/includes/class.llms.template.loader.php#L76) and write something like:

$is_redirected = llms_is_page_restricted($course->get('id')) && $course->has_sales_page_redirect();

and in this case display an empty content. For the other posts type I think we should make sure to return an empty post content + any restriction message, through llms_get_restriction_message().

This means that I'm going to repeat some core logic in the rest-api codebase, as I don't see a way to proficiently re-use the current code in the template loader class for this purpose.

Well, I wanted to make you aware of the direction I'm taking here, stop me if you think otherwise :D

[thomasplevy]

@eri-trabiccolo

I think this is a good direction and seems less complicated now that you've worked through it a bit.

As far as redeclaring logic I'm not sure I see any better way to do it either. I don't really see a way to refactor the core to make eliminate this redundancy but if you come up with any way we can bundle some of this logic into new core functions to reduce the redundancy I'm all for handling that way.

If there is a bit of redundancy that's acceptable. Add an @todo near the redundant sections noting where the redundant code is in the core and in the future we can tackle a refactor if possible.

[eri-trabiccolo]

Hi @thomasplevy I'm working on this branch https://github.com/eri-trabiccolo/lifterlms-rest/tree/restrictions If you're interested on the ongoing development of this and you have the time take a look at the LLMS_REST_Posts_Controller::maybe_restrict_object_data() method where I've centralized the restrictions handling. This is not ready to merge as I have to fix some stuff before. Will open a PR once done.

[thomasplevy]

@eri-trabiccolo please remind me about this on Monday if I forget

[eri-trabiccolo]

@thomasplevy I hope a will have a working PR for Monday! :)

[thomasplevy]

It looks pretty sound to me. I read through it pretty quickly but it's pretty clear and seems to do what it looks like it should do (despite it being some unfortunately complicated logic to make it all work).

I'll do a full review on it after you submit the PR sometime next week.

I think it's solid though, nice work

[eri-trabiccolo]

---

For Thomas: please don't dive into this right now, I'll ping you in the coming days: This comment can be broken in more than one issue, but I needed to write my thoughts on the matter in one place first as it's all connected (at least in my brain).

---

We should empty the audio_embed and video_embed properties (for those post types who have them) as well when a post type is redirected (restricted): think about a video course, a restricted lesson cannot "expose" the video embed, would make no sense.

Does this mean that the audio_embed and video_embed properties should have the 'restricted' sub-property as well? how should they look like then? Something like the following?

'audio_embed' => [
   'raw' => {url} (edit)
   'rendered'(*) => {url}|'' (edit/view)
   'restricted' (ro) => false|true (edit/view)
]
// (*) or something similar, because the rendered would rather be the result of the embed

Please excuse the crudity of this model. I didn't have time to build it to scale or paint it. E. Brown

I think we should for consistency... also the consistency would allow us to automatically retrieve the properties to be restricted from the schema (something that I've actually already implemented in my latest commit)... What do you think?

Also note that the restricted property, even if similar to the wp core protected one, behaves a little bit different: the protected sub-property is true if the post has a password associated, meaning that that value doesn't depend on the user caps (see also edit context) but reflects an intrinsic property of the post. This is not true for the restricted property, as it relies on the llms_page_restricted() llms core function which depends on if the user is enrolled in a course/membership and on the user caps/role (see the filter applied by the view manager that uses llms_can_user_bypass_restrictions()).

BTW do you think we should disable the view manager in rest calls? Would make sense IMHO.

[eri-trabiccolo]

@thomasplevy so in the latest days, I kinda slowly back on working on this, I'm going to write tests on what I've done until now: which is basically what we discussed - add the restricted sub-property for the llms posts content and excerpt properties + handle the restrictions mostly mimicking the core's template loader. When you got time could you take a look at my comment above? My PR won't depend on this, is something we can add later. Thanks!

[thomasplevy]

@eri-trabiccolo

Your "crude" seems good to me.

This is also a model we could very likely use elsewhere in the codebase as well.

BTW do you think we should disable the view manager in rest calls? Would make sense IMHO.

Do you mean the class the restriction function causing the redirects / filters to remove the post content?

Yes. I think that would make sense as long as all our rest controllers are properly checking permissions to ensure that restricted content isn't leaked through REST endpoints.

Finally: this will have to be a backwards incompatible change. Which I believe is okay as we're in beta and API changes are expected and noted in the documentation. We'll have to note as much in the changelog but this is a superior design and should be implemented.

[eri-trabiccolo]

@thomasplevy

Do you mean the class the restriction function causing the redirects / filters to remove the post content?

Yes. I think that would make sense as long as all our rest controllers are properly checking permissions to ensure that restricted content isn't leaked through REST endpoints.

I mean that, in rest calls, we should not load this class at all: https://github.com/gocodebox/lifterlms/blob/master/includes/class.llms.view.manager.php and add proper filters to handle the restrictions based on the caps + context (edit).

Finally: this will have to be a backwards incompatible change. Which I believe is okay as we're in beta and API changes are expected and noted in the documentation. We'll have to note as much in the changelog but this is a superior design and should be implemented.

Yep, because audio and video properties will change their data structure.

Thanks for the feedback man, much appreciated!

[thomasplevy]

Oh, yea, kill the view manager in rest for sure