Resource Feedback for Courses, Students, Enrollments about lifterlms-rest HOT 16 CLOSED

Shall we expose the password field? I think we should not, I think we should allow its creation/update (so context=edit) only.

Good catch, see new issue to track: #9

Access plans: Maybe we can add access plans ids and/or add them to the _links array as well.

Per #1 access plans will be their own resource at /plans -- once that resource is defined then courses will have an _link to the collection of relevant plans.

In the current state of LifterLMS plans belong to a course or membership but in the future we'd like to have global plans that can be shared. Moving them to their own resource in the rest api (instead of nested) will not require us to add global plans today but in the future when we do it'll be an easy transition on the api side.

What about Sections and Lessons: shouldn't we add them to _links?

Yes. As above, I'd like sections/lessons/quizzes/etc (collectively going to refer to them as "content" to be their own resource. See #1 again, the resource will be courses/{id}/contents

Links will be added to the course schema too when that's specced out.

Since we have the comment(ping)_status we can also add the review_status. Probably we can either add comments/ping/review links as related resources.

Nice catch, I completely forgot about reviews (as is often the case). See #10

I see you removed the date_gmt property for creation/update methods. I think we should add it back, otherwise wp_insert_post it can retrieve it from the date but only if the data contains informations about the gmt "offset"... otherwise data_gmt will be equal to date. Does this make sense to you? If so, let's reintroduce it :D

Oh yea, you're right, see #11

from lifterlms-rest.

@thomas
I see what you mean yes, thanks.

1. I missed the read-only "mark" (though I guessed it :D), sorry, I was looking at the html not the json.
2. No need to be enum then, it's "fixed", and it's the current "object" post_type.

from lifterlms-rest.

A sort of "capacity status" should be added, we have the course's enable_capacity bool.

Added as capacaity_enabled, see 7b9712b#diff-f438ec0990118ddfa46daae2716d8ff2R67

from lifterlms-rest.

Thanks, I think my editor is buggy, I searched for any "remove_action"/"remove_all_" throughout the lifterlms-blocks repo and couldn't find them.
Good to know I cannot trust it, will go with grep next time :D

from lifterlms-rest.

@eri-trabiccolo & @actual-saurabh can you please review and let me know if you find any holes / issues / whatever

from lifterlms-rest.

I start with some initial considerations on Courses:

• Shall we expose the password field? I think we should not, I think we should allow its creation/update (so context=edit) only.
• Access plans: Maybe we can add access plans ids and/or add them to the _links array as well.
• What about Sections and Lessons: shouldn't we add them to _links?
• Since we have the comment(ping)_status we can also add the review_status. Probably we can either add comments/ping/review links as related resources.
• I see you removed the date_gmt property for creation/update methods. I think we should add it back, otherwise wp_insert_post it can retrieve it from the date but only if the data contains informations about the gmt "offset"... otherwise data_gmt will be equal to date. Does this make sense to you? If so, let's reintroduce it :D

from lifterlms-rest.

About the Course's access period object:
opens and closes message field in the specs contain shortcodes...
This makes me thing that we should split all the properties which can contain shortcodes (or blocks?!) into raw and rendered with the second only accessible in edit context, exactly as we do with the title,content,excerpt.
What do you think?

from lifterlms-rest.

I don't think I get the meaning of the "post_type" property for the Course resource. :D
Enum:"course" "llms_membership".

The course post_type can be only "course" no?!

from lifterlms-rest.

@eri-trabiccolo

I don't think I get the meaning of the "post_type" property for the Course resource. :D
Enum:"course" "llms_membership".

The course post_type can be only "course" no?!

In the /courses resource it doesn't make sense, exactly, no. However when pulling courses as a link from other resources via link (like the /enrollments "post" resource the post could be either a course or membership.

Unless we split every resource that can have either courses or memberships into separate resources (/course-enrollments and /membership-enrollments for example) having the post_type property on course and memberships makes sense.

Additionally, following patterns from the WP Core, the post schema for any post type returns its post type as a parameter even when pulling it from the post type resource. See type in the return of a /wp-json/wp/v2/posts request, for an example

I'm open to solving this a different way but I think in this context it makes sense. The alternative I can come up with would be to add _links to the post type information. However, this would require additional API calls or parsing to determine the post type instead of just reading the text field.

Edit:

Also note that the post_type field is marked as read only for the purposes of our API resources. When creating/updating courses (and later memberships after we define that spec) you won't have to (and will not be able to) specify a post type. You'll only be able to read it during gets

from lifterlms-rest.

A sort of "capacity status" should be added, we have the course's enable_capacity bool.

from lifterlms-rest.

@thomasplevy I have a "problem" :D
First I want to be sure how the Course "rendered" content should look like. Let me explain.

If we built the Course content with the block editor, stuff like the instructor and the syllabus are part of the rendered content. While if the course has been built with the "classic editor" instructor, syllabus etc. are rendered with filter applied to the content hook (well a filter that loads before and after templates that will trigger action hooks etc. etc.)

Should the Course's rendered content always display such "contents"? I guess so...
Hence, the 'rendered' property would be:
apply_filters( 'the_content', $post->post_content );

And here's my problem:
If I use the code above, for some reason, the rendered content of a course built with the block editor has "pieces" like instructor, syllabus etc. duplicated. The duplication comes from the template hooks: e.g. add_action( 'lifterlms_single_course_after_summary', 'lifterlms_template_course_author', 40 );

I think these kind of actions are removed in the core when a course has blocks but I cannot find the code so that I can trigger it when building the rest response.

Any hints?

from lifterlms-rest.

I think these kind of actions are removed in the core when a course has blocks but I cannot find the code so that I can trigger it when building the rest response.

See https://github.com/gocodebox/lifterlms-blocks/blob/c006aa3056d8e6e4bb40881a15bc5949da58a293/includes/class-llms-blocks-migrate.php#L249

Since this action is run at wp it's possible that it's not being run during rest_api_init -- I'm guessing. Look into the action stack and see if that could be causing it.

If it is, I think the rest api plugin should duplicate this effort for now and also remove these actions like the blocks plugin does. I think.

If this doesn't solve it open this as a new issue so we can track it more precisely

from lifterlms-rest.

On second thought i think it'd be better to update this in the blocks plugin so that we don't need to duplicate all the existing logic there to determine if a post is migrated.

There could just be an additional action in the constructor to run the same remove action method in the blocks plugin

from lifterlms-rest.

Mmm I think the problem here would be when retrieving the collection, the actions "removal" should be done for each course: I should remove those actions if the current post is migrated, but don't remove those if it is not, which means potentially re-adding them for not migrated posts. Which also means that I should check an action was actually hooked, otherwise I'll end up adding actions someone removed.
Do you still think this should be addressed in the blocks plugin?
Hope I've been understandable.

from lifterlms-rest.

On second thought i think it'd be better to update this in the blocks plugin so that we don't need to duplicate all the existing logic there to determine if a post is migrated.

There could just be an additional action in the constructor to run the same remove action method in the blocks plugin

A per my previous comment I've implemented it in this repo.
Basically when preparing the object (e.g. single course, or a course in a collection) for the response, I do the following:

1. get a list of action/filters to be removed for the current object (a course, a membership and a lesson have a different list)
2. remove them and keep track of what I removed
3. prepare the object for response (e.g. get the rendered content along with the other properties)
4. re-add the previously removed filters.

from lifterlms-rest.

@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?

from lifterlms-rest.