Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: 404 in favor of 400 in progress controller #185

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

Conversation

thomasplevy
Copy link
Contributor

@thomasplevy thomasplevy commented Jul 27, 2020

This is a early WIP to per #181

(I named the branch wrong)

@eri-trabiccolo can you have a look through the changes here and see if I'm track per our discussion in #181

I'm submitting this early because I want to make some progress on this but I'm feeling uncertain whether I'm moving in the right direction.

This still requires the following before it can be submitted as a full PR and merged:

  • Tests for new server method llms_rest_page_restricted()
  • Logic for GET requests on the progress controller per discussion in Student progress should return a 404, not a 400 validation error, when requesting progress for an invalid post id #181
  • A review of the existing DELETE requests -- I believe they're solid but they may need updating
  • A review of the generated links. If a 3rd party adds a completeable post type and uses this endpoint to record completion the resulting links will (potentially) lead to 404s
  • Tests to simulate progress recording by a 3rd party for a custom post type (we can filter to allow completion of a page and see what happens. It should work).

Comment on lines +90 to +96
// Store preexisting global values.
$temp_post = $post;
$temp_is_singular = $wp_query->is_singular;

// Override them.
$post = get_post( $post_id );
$wp_query->is_singular = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following this approach on my branch https://github.com/eri-trabiccolo/lifterlms-rest/tree/restrictions
eri-trabiccolo@debe8b5#diff-5ce7aeb33633a2220e61908290ff5aedR806-R815

I needed to set the query's queried_object and the queried_object_id as well, I cannot remember exactly why at the moment. But just in case you'd find some inconsistencies during your tests, take this into account.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I saw you start doing something like this somewhere but I couldn't find it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the reason I added this as a function was because I was pretty sure you were doing something like this and figured we'd need it elsewhere at somepoint

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you did it right.
I did it inside the "loop" in the post controller because I needed it in the posts loop but I will adapt my code using the new function. Looks more clean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eri-trabiccolo I'm working on reworking llms_page_restricted() to be useable outside a loop. This function is some very stringy code and needs a refactor. I'm taking it on instead of writing a hack around it.

@eri-trabiccolo eri-trabiccolo marked this pull request as draft April 28, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants