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

Add REST API endpoint to read/update general settings #2695

Merged
merged 14 commits into from Dec 17, 2018
Merged

Conversation

Shelob9
Copy link
Collaborator

@Shelob9 Shelob9 commented Aug 7, 2018

#2694
Endpoints added via this PR:

  • POST /wp-json/cf-api/v2/settings - Update general settings.
  • GET /wp-json/cf-api/v2/settings - Read general settings.

@Shelob9 Shelob9 added the PR (Patch) Is a pull request label Aug 7, 2018
@Shelob9 Shelob9 added this to the 1.7.3 milestone Aug 7, 2018
@Shelob9 Shelob9 self-assigned this Aug 7, 2018
@Shelob9 Shelob9 modified the milestones: 1.7.3, 1.7.4 Sep 26, 2018
@Shelob9 Shelob9 modified the milestones: 1.7.4, 1.8.0 Oct 19, 2018
@Shelob9 Shelob9 requested a review from New0 November 19, 2018 19:46
@Shelob9
Copy link
Collaborator Author

Shelob9 commented Nov 19, 2018

@New0 Please review this and merge if you approve. Having this and #2692 in 1.8.0 will make working on admin refactor easier.

@New0
Copy link
Collaborator

New0 commented Nov 30, 2018

@Shelob9 I didn't find PHP unit tests for the new routes so I started testing manually.

  • In classes/api/settings.php file, the 'permission_callback' declared for the new routes is not written (l23, l36, l56)
    • I tried to edite class declaration l12 to 'extends Caldera_Forms_API_CRUD' instead of 'implements Caldera_Forms_API_Route' and used 'get_items_permissions_check' but had result {"code":"rest_forbidden","message":"Sorry, you are not allowed to do that.","data":{"status":401}}

This test throws an error right now, the permissions_callback parameter seems to create this issue.
I had started to write a whole class tha set up a mocked $wp_rest_server when I realized we had it already created at tests/includes/cf-rest-test-case.php and could extend CF_Rest_Test_Case
…aldera_Forms::get_manage_cap() capability

- I first tried to use Caldera_Forms_API_CRUD::get_items_permissions_check but it uses the _doing_it_wrong() function and I'm not sure it's what we want here
- the permissions_check function check if the user is set. If not, we look for the user using the determine_current_user filter hook to go forward when user is set and find the user ID ( if logged in ) and the set the user again.
- we might want to move the permissions_check function to classes/api/crud.php in the case @Shelob9 agrees with it
This test the responses of api requests
@Shelob9 I couldn't test post requests using args, right now that broke the tests
Fixed testing with postman,
 @Shelob9 for context : I'll go back to render/index.js refactor before I look for a solution to test POST requests with args
Use get_items_permissions_check and create_item_permissions_check with custom create or view filters for settings
public function get_items_permissions_check( WP_REST_Request $request ){

//Check if a user is already set
if( wp_get_current_user()->ID === 0 ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@New0 Please remove this, like we discussed.

*
* @return bool
*/
public function permissions_check(){
public function create_item_permissions_check( WP_REST_Request $request ){

//Check if a user is already set
if( wp_get_current_user()->ID === 0 ) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@New0 @New0 Please remove this, like we discussed.

@New0 New0 merged commit ba75451 into develop Dec 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR (Patch) Is a pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants