typo: preform => perform :P

Originally posted by @eri-trabiccolo in #34

I think we have to "define" how much nested our fields can be, for two reasons:

1. rules :D which means "practical in the future"
2. "practical now"
   There's a core helper that, given a response data (array) and a context (and implicitly a schema), will filter the "response" by context:
   https://github.com/WordPress/WordPress/blob/5.2.2/wp-includes/rest-api/endpoints/class-wp-rest-controller.php#L233
   As you can see it will only work with a maximum of one nesting level (it will work with the 'content' => ['raw','rendered'] structure type).

I happen to use it, of course we can override it, and we'll fix the point 2), but are we sure it'll be a good choice for the future?

Of course I see that helper is kinda limited and we can possibly do it better.

Add logic to handle access to password protected llms posts.

We can look at how the WordPress core does in wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php

See #6 (comment)

https://gocodebox.github.io/lifterlms-rest/#tag/Courses/paths/~1courses~1{id}~1content/get

• The parent arg doesn't make sense to me there, as we're already in the parent context.
• The exclude arg is missing... why?
• There's also the password arg, that in this case refers to the parent course, I guess. We've talked about the password protection inheritance but didn't come up with a definitive decision yet, right?!

enrollment should be removed from orderby

And include parameter should be replaced with student_id here:
https://gocodebox.github.io/lifterlms-rest/#tag/Courses/paths/~1courses~1{id}~1enrollments/get
(course's enrollments)

I guess student_id should still accept a single id or a comma separated list of ids.

By default set to false.

This arg determines whether to bypass trash and force deletion.

Phase 1 (Completed)

Resources to be made available in the first phase of the API. Target public release August 2019.

• Student accounts /students
• Student enrollments /students/{id}/enrollments
• Student Progress /students/{id}/progress
• Instructors /instructors
• Instructor Content /instructors/{id}/content
• Courses /courses
• Course Enrollments /courses/{id}/enrollments
• Course Instructors /courses/{id}/instructors /instructors?post={id}
• Course Contents (syllabus/outline) /courses/{id}/contents
• Sections /sections
• Lessons /lessons
• Quizzes /quizzes
• Quiz Questions /questions
• Enrollments /enrollments
• Memberships /memberships
• Membership Enrollments /memberships/{id}/enrollments /enrollments?post={id}
• Access Plans /access-plans
• Webhooks

Ecommerce

• Orders /orders
• Transactions /orders/{id}/transactions
• Refunds /orders/{id}/refunds
• Coupons /coupons

Engagements

• Engagements
• Achievements
• Certificates
• engagement emails

Grades & Quizzes

• Course Grades /courses/{id}/grades
• Quiz Attempts /quizzes/{id}/attempts

System & Status

• Settings /settings
• Integrations
• Payment Gateways
• System Status
• Reports (sales, enrollments, etc...)

Other

• Reviews /reviews/ [#10]
• Vouchers /vouchers
• Notifications /notifications
• Events /events - See gocodebox/lifterlms#636

Add-Ons

Assignments

• Assignments /assignments
• Assignment Tasks /assignments/{id}/tasks
• Assignment Submissions /assignments/{id}/submissions

Social Learning

(todo)

Private Areas

• Private posts /private-posts (CRUD)
• Private Post Comments /private-posts/{id}/comments (possibly unnecessary due to commenting api available via the WP core).
• Automations /private-post-automations/ (CRUD)

Advanced Quizzes

• Additional question types

while it's still /wp-json/llms/v1/enrollments?post=1234

This variable is defined but never used. I'm not user if this variable is intended to be used instead of the callback reference on line 116 (

We should be able to filter the lessons at least by parent section.
This is "suggested" by the siblings link:

Since not all tests will require tests of actual REST API methods run via the actual REST API server I think a second test case should be created which extends this test case and then adds these this setUp() and tearDown() methods.

This test case could be LLMS_REST_Server_Unit_Test_Case

Then other tests (functions, install, custom post types, etc...) can use this generic test case and the API Server classes / controllers / etc can extend the server class which spools up and down the server reference.

yes|no properties:

• (access) time_period (string) Whether or not a course time period restriction is enabled [yes|no] (all checks should check for 'yes' as an empty string might be returned)
• enrollment_period (string) Whether or not a course time period restriction is enabled [yes|no] (all checks should check for 'yes' as an empty string might be returned)
• has_prerequisite (string) Determine if prerequisites are enabled [yes|no]
• tile_featured_video (string) Displays the featured video instead of the featured image on course tiles [yes|no]

others:

• average_grade (float) Calculated value of the overall average grade of all enrolled students in the course.
• average_progress (float) Calculated value of the overall average progress of all enrolled students in the course.

Allow easy viewing of each object/model/schema like most API references. For example:

Stripe
WooCommerce

Currently tracking a PR in ReDocly that should make this pretty simple: Redocly/redoc#681

We could also do this by adding custom HTML to each tag description

@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 #6 (comment)

• Listing endpoint (get_items)
• Complete tests

For example, if updating a course's content, the api should accept content as the parameter, the request should not require a submission of raw/rendered data via passing content.raw and/or content.rendered

Raw content should always be supplied directly to the main property. The object is passed during GET requests.

Add context parameters for all requests.

Available contexts:

view (default) return content for viewing/displaying
edit returns content with additional "raw" data used for displaying the content in the WP Editor for editing.

Post fields should make use of this by returning certain fields as an object with additional data depending on the context.

EG: GET /courses/{course_id} would return in view context:

{
   "content": {
       "rendered": "lorem ipsum dolor sit"
   },
   "title": {
       "rendered": "lorem ipsum dolor sit"
   },   
}

The same resource in edit context would return:

{
   "content": {
       "rendered": "lorem ipsum dolor sit"
       "raw": "lorem ipsum dolor sit"
   },
   "title": {
       "rendered": "lorem ipsum dolor sit",
       "raw": "lorem ipsum dolor sit"
   },   
}

Fields with raw and rendered values include:

post content, post title, post excerpt.

More to come

• List
• Create
• Retrieve
• Update
• Delete
• List Content
• Test Coverage 90% or greater

The API will implement HTTP Basic Authentication (consumer key/secret) via request headers.

As a fallback for servers which cannot properly parse request headers the consumer key/secret may be passed as query string variables, for example POST /students?consumer_key={$key}&consumer_secret={$secret}

The API will only accept HTTPS requests.

API keys will be stored in a custom table, wp_lifterlms_api_keys with the following structure:

  `key_id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `user_id` bigint(20) unsigned NOT NULL,
  `description` varchar(200) COLLATE utf8mb4_unicode_ci DEFAULT NULL,
  `permissions` varchar(10) COLLATE utf8mb4_unicode_ci NOT NULL,
  `consumer_key` char(64) COLLATE utf8mb4_unicode_ci NOT NULL,
  `consumer_secret` char(43) COLLATE utf8mb4_unicode_ci NOT NULL,
  `last_access` datetime DEFAULT NULL,
  PRIMARY KEY (`key_id`),
  KEY `consumer_key` (`consumer_key`),
  KEY `consumer_secret` (`consumer_secret`)

Each key will belong to a WP_User (related via user_id)

Permissions are an enum: read, write, read_write, this context will be taken into account during requests. EG: a key with read permissions will receive a 403 Forbidden response when attempting to create a resources and a key with write permissions will receive a 403 Forbidden when attempting to read resources. A read_write key will have full permissions.

Furthermore, user permissions and capabilities will define the access. If an instructor is provided with an API key, they will not be able to preform updates on resources they don't have access to outside of their capabilities.

Requests made to the API using cookie authentication will authenticate via the current user and provide that user with resource access based off their user roles and capabilities.

An interface (within the WordPress admin panel) will be created to manage (CRUD) API keys.

Do you think would be worthwhile adding links to the previous/next section resource under _links?

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?

Originally posted by @eri-trabiccolo in #6 (comment)

1. Should we allow date creation customization?

The core api do not really provide this... as far as I can see.
Should we implement the code for the rest api only?

Shouldn't only allow for enrollments status changes?!

2. Should we allow 'custom' status on creation?
   Does it make sense create an enrollment for a student/course with a cancelled or expired status?

See #6 (comment)

href: /wp-json/llms/v1/students?enrolled_in={course_id}

The spec defines a 204 status code for all deletions and an empty response body, eg: https://gocodebox.github.io/lifterlms-rest/#tag/Courses/paths/~1courses~1{id}/delete

It looks like we're currently mimicking the behavior of the WP Core's WP_REST_Posts_Controller and responding with a 200 as well as a body containing information about the deleted post.

I'm open to adjusting this to be a 200 with the existing response body. However, in researching the spec most apis (outside of WP) do not respond with a body when deleting a resource.

Additionally, I came across this SO thread: https://stackoverflow.com/questions/6439416/deleting-a-resource-using-http-delete

This essentially says that you should never return a 404 on a deletion request. And should always return a 204 (or 200). If we were going to return a body we would not be able to return the previous content if we go with this stance. EG: If the resource never exists (and we still return the 200/4 then we wouldn't be able to pull the deleted resource's information.

I'm leaning towards switching up the current courses to be a 204 with no body. Do you agree?

Is there a good reason I'm missing why we need to return the deleted post in the request body?

The best thing I can come up with is that a client wants to "check the work of the api" against the deleted content but they shouldn't ever really need to do that (I don't think). If we tell them it's deleted that's enough. And if for some reason they need the deleted posts information they should preform a GET first, do whatever they need, and then delete.

Right?

"borrow" from WooCommerce:

public function generate_signature( $payload ) {
	$hash_algo = apply_filters( 'woocommerce_webhook_hash_algorithm', 'sha256', $payload, $this->get_id() );
	return base64_encode( hash_hmac( $hash_algo, $payload, wp_specialchars_decode( $this->get_secret(), ENT_QUOTES ), true ) );
}

Source: https://github.com/woocommerce/woocommerce/blob/master/includes/class-wc-webhook.php#L470-L483

This solution will not prevent against "replay" attacks, see Stripes solution: https://stripe.com/docs/webhooks/signatures#replay-attacks

Our method can be improved to prevent against replays by adding a timestamp to the generation and payload:

public function generate_signature( $payload ) {
	$hash_algo = apply_filters( 'llms_webhook_hash_algorithm', 'sha256', $payload, $this->get_id() );
	$ts = current_time( 'timestamp' );
	$message = $ts . '.' . $payload;
	return 'ts=' . $ts . ',v1=' . hash_hmac( $hash_algo, $message, wp_specialchars_decode( $this->get_secret(), ENT_QUOTES ), true );
}

X-LLMS-Webhook-Signature: t=1492774577,v1=5257a869e7ecebeda32affa62cdca3fa51cad7e77a0e56ff536d0ce8e108d8bd

This will ditch the base64 encoding and add a timestamp which can be extracted and used to generate an expected signature.

The signature header would something like:

See:

https://github.com/gocodebox/lifterlms-rest/blob/master/spec/components/schemas/SectionResponse.yaml#L48

It should be:
/wp-json/llms/v1/sections/987/content

right?!

P.S.
since you're there, small typo prevous -> previous

shouldn't the student_id and post_id be read-only?

• List
• Create
• Retrieve
• Update
• Delete
• List Content
• List Enrollments
• Test Coverage 90% or greater

Open discussion thread for discussion of these resources as documented in the current state of the spec.

Let's pay special attention to:

1. The properties available on each resource
2. The _links provided for each resource to help consumers gather additional information about related resources.

https://gocodebox.github.io/lifterlms-rest/

See #6 (comment)

The WP rest api adds the sub property protected (bool) to the content and excerpt properties.
The reason behind this is that those properties, when a post is password protected, are returned as empty string, so, that there's no way to discriminate between an empty content and a protected content (in view context).
I think we should add this sub property to the specs as well.
Of course it should be added to all the properties that we want to be affected by the password protection (which at the moment are only content and excerpt).

What do you think?

https://gocodebox.github.io/lifterlms-rest/#tag/Sections/paths/~1sections/get

orderby: menu_order shouldn't be there, while order should be there as it makes sense when filtering by parent.

Right?

(There's another occurrence of the show_in_rest filtering but it's about updating permission, so it might even stay the way it is right now.)