gocodebox / lifterlms-rest

LifterLMS REST API Feature Plugin
6 stars 7 forks source link

List membership enrollments always returns 404 #178

Open GeekJosh opened 4 years ago

GeekJosh commented 4 years ago

Reproduction Steps

Perform a GET request to the /memberships/{id}/enrollments endpoint using any valid membership post id

Expected Behavior

A collection of enrollments for the given membership is returned

Actual Behavior

The API returns 404 llms_rest_not_found

Error Messages / Logs

{
    "code": "llms_rest_not_found",
    "message": "The requested resource could not be found.",
    "data": {
        "status": 404
    }
}

System Information

System Report ``` LifterLMS Rest API 1.0.0-beta.12 ```
GeekJosh commented 4 years ago

after realising that using the X-LLMS headers fixes the issue I reported in #179 I have just confirmed the same is true here. It seems that somewhere, Basic Auth header isn't being handled properly

thomasplevy commented 4 years ago

~See my reply to your other issue at https://github.com/gocodebox/lifterlms-rest/issues/179#issuecomment-653139278~

~I think we have duplicate issues here~

thomasplevy commented 4 years ago

I misread the title when reviewing the other issue and thought this was stating the the GET /memberships endpoint was the reported problem endpoint.

This actually seems to me to be completely unrelated to #179 (it's a 404 not a 401/3) and I am seeing issues here. It is 404ing regardless of whether or not basic auth or header auth is used.

Could you test you integration again and let me know what kind of result you're getting when using header auth.

You should be getting a 404 either way

GeekJosh commented 4 years ago

Using header Auth, I get the collection of enrollments as expected

thomasplevy commented 4 years ago

@GeekJosh hmmm...

I'm seeing a different issue and what you're reporting is very strange in this case.

Could you post some example requests you're making so I can look at your integration a little closer please?

thomasplevy commented 4 years ago

@GeekJosh

Using header Auth, I get the collection of enrollments as expected

Is it possible you're testing against different memberships?

I found the following:

https://github.com/gocodebox/lifterlms-rest/blob/d9ec7708bc8365a5b9ddc5b11104f4a934764542/includes/server/class-llms-rest-enrollments-controller.php#L210-L213

Which in some capacity I know I must have read and reviewed but I can't find anything in the spec stating enrollment lists should respond with a 404 when no enrollments are found. So I think this is the bug we're having here.

This means that both basic auth and header auth should respond with a 200 for a membership with at least one enrolled student and should respond with a 404 for a membership with no enrollments.

Could you confirm if this is the case?

@eri-trabiccolo could you use your infallible memory to dredge up the conversation we had in which I told you to make this respond with a 404 (and then never documented as much) and why? Or is this a bug?

Looking at this today it feels like it should respond with an empty list (not a 404) but I don't recall the context of why we decided to make it respond with 404.

If it is a bug we should fix and if there's a good reason we decided on this (which again, I've forgotten) we need to update the spec to make this explicitly clear.

eri-trabiccolo commented 4 years ago

@thomasplevy that might actually be a bug but I implemented that way on purpose because that's what the spec say: https://developer.lifterlms.com/rest-api/#tag/Memberships/paths/~1memberships~1{id}~1enrollments/get

similar thing for the courses.

You already know the spec say that, but you have to imagine that I implemented most of the resources based on what the spec said.

thomasplevy commented 4 years ago

Can you clarify where you're seeing that the spec says to return a 404 when the membership has no enrollments?

The 404 I see documented I think would be expected when the membership doesn't exist -- not when no enrollments exist, right?

eri-trabiccolo commented 4 years ago

Mmm well no to me it meant that the enrollments did not exist, so it's my wrong understanding of the specs: BUG. Maybe the spec could be more "complete" if they said what was the actual resource that couldn't be found.

I did the same with:

they must be fixed then! Sorry!

thomasplevy commented 4 years ago

Aha!

thomasplevy commented 4 years ago

@GeekJosh in light of this bug that we've identified can you triple check that your initially reported experience and behavior is accurate?

Can you clarify where you're seeing that the spec says to return a 404 when the membership has no enrollments?

The 404 I see documented I think would be expected when the membership doesn't exist -- not when no enrollments exist, right?

I think that you have misreported the issue as an auth issue but in fact what we have is a bug in our code returning a 404 instead of a 200 with an empty array

thomasplevy commented 4 years ago

@GeekJosh can you check in with me on my last comment. We have found a bug (but it's unrelated to your initial report) and I'd like some clarification from you before closing this out.

GeekJosh commented 4 years ago

Sorry for the delay - I can confirm that regardless of using header/basic auth, I get a 404 on an empty collection

thomasplevy commented 4 years ago

@GeekJosh thanks for confirming