access plan `availability_restrictions` property never returned

gocodebox / lifterlms-rest

LifterLMS REST API Feature Plugin

6 stars 7 forks source link

access plan `availability_restrictions` property never returned #269

Closed thomasplevy closed 2 years ago

thomasplevy commented 2 years ago

Reproduction Steps

Create an access plan with membership restrictions stored in the availability_restrictions property.
Retrieve the access plan via a GET /access-plans/$id request or a GET /access-plans (list) request that would return the access plan with restrictions

Expected Behavior

The availability_restrictions property contains an array of WP_Post IDs

Actual Behavior

An empty array is returned.

Looks like the error is here:

https://github.com/gocodebox/lifterlms-rest/blob/a3a644da3edd10d923e967fe0b6244606d3d1f5f/includes/server/class-llms-rest-access-plans-controller.php#L278

the empty array is in the wrong position on the ternary

Also after the above is corrected, ideally an array of ints should be returned instead of an array of strings

thomasplevy commented 2 years ago

@eri-trabiccolo I've already confirmed this and written a quick fix locally so I'm not blocked but this could be cleaned up sooner than later for release with the other access plan bug you already fixed

eri-trabiccolo commented 2 years ago

@thomasplevy

Also found that we don't have an availability schema property, but the model's has_availability_restrictions() relies on that. So I'm going to switch the availability based on whether availability_restriction property has been provided.
Also the availability thing is only for access plans related to courses. I think we should notice this in the documentation (and the schema), and possibly don't return availaibilty_restrictions props for memberships.

Agreed?

thomasplevy commented 2 years ago

I think this makes sense @eri-trabiccolo. The property barely seems necessary today and really seems unnecessary with what I'm building right now. So keeping it as you have for backwards compatibility purposes but not requiring users to have to actually update it via REST feels good.

I don't want to take the time to properly deprecated it right now but we can phase out it's usefulness and maybe remove it officially alongside the release of the react stuff I'm working on