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

convert admin ajax endpoints to WP-API endpoints #42

Closed
wants to merge 20 commits into from

Conversation

pgk
Copy link
Contributor

@pgk pgk commented Jan 6, 2016

Refs #40: Convert admin AJAX endpoints and zone feed to WP-API endpoints

Created Zoninator_Rest_Api_Controller class to represent the new rest API. Amended js code to use the new endpoints.

Endpoints

The following endpoints where created

  • POST /zoninator/v1/zones/:zone_id/posts (add post to zone)
  • GET /zoninator/v1/zones/:zone_id/posts (get zone feed)
  • GET /zoninator/v1/posts/search?term=:term (search term)
  • DELETE /zoninator/v1/zones/:zone_id/posts/:post_id (remove post from zone)
  • PUT /zoninator/v1/zones/:zone_id/posts/order (reorder posts)
  • PUT /zoninator/v1/zones/:zone_id/lock (lock zone)
  • GET /zoninator/v1/zones/:zone_id/posts/recent (recent posts for select box)

NOTE: Supplemented the zone feed endpoint with a RESTful endpoint. Did keep the
old zone feed rewrite rule though, as it might be used by third parties.

Other

Refactored Zoninator class in the following ways

  • extracted zone_gateway class
  • extracted common aspects of view rendering into separate class
  • extracted permission checking used both by Zoninator_Rest_Api_Controller and Zoninator into separate class
  • Created PHPUnit tests for Zoninator_Rest_Api_Controller
  • Created a script to automate the test env setup (assumes Ubuntu and bash)
  • removed warnings, did some style fixes

Refs #40: Convert admin AJAX endpoints and zone feed to WP-API endpoints

Also created Zoninator_Rest_Api_Controller and refactored zoninator into separate
classes. Added unit tests for Zoninator_Rest_Api_Controller.
@danielbachhuber
Copy link

You might want to take a look at the pattern we've established for core endpoints. If you follow it, you'll be able to benefit from the abstractions we're creating.

@pgk
Copy link
Contributor Author

pgk commented Jan 7, 2016

Thanks @danielbachhuber will refactor the Controller class to follow the established pattern. It is sensible to use it's abstractions, especially for (GET, POST and DELETE) on /zoninator/v1/zones/:zone_id/posts and possibly for the rest.

Instead, deactivate and gracefully inform the users to either

* upgrade their WordPress
* downgrade Zoninator to v0.6
break;
}

if( ! call_user_func( array( $this, $verify_function ), $zone_id ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we just call the method directly within the switch rather than using call_user_func? Feels like that would make it easier to read/follow.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see it's just a copy-paste of the original function. I think this gives us an an opportunity to improve the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

data = {};
}

zoninator.$zonePostSearch.trigger( 'zoninator.ajax', [ endpoint, method, data ] );
Copy link
Member

Choose a reason for hiding this comment

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

This could be a back-compat issue as well as the original trigger allowed sites to customize the params sent via the AJAX endpoint (e.g. custom search params). Any thoughts on how we could handle this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ad768c7

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.

3 participants