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

Chrome: Adding the trash post link to the sidebar #862

Merged
merged 3 commits into from
May 24, 2017

Conversation

youknowriad
Copy link
Contributor

This PR adds a trash post link to the sidebar.

Open questions

  • What do we do after removing the post? I'm redirecting to a fresh Gutenberg page right now.

@youknowriad youknowriad added the General Interface Parts of the UI which don't fall neatly under other labels. label May 22, 2017
@youknowriad youknowriad self-assigned this May 22, 2017
@jasmussen
Copy link
Contributor

Looks good!

Though I'm getting a 405, rest route not allowed JS error when clicking on it:

screen shot 2017-05-22 at 15 19 11

What do we do after removing the post? I'm redirecting to a fresh Gutenberg page right now.

In WP-admin right now you're moved to the posts list, where you see an Undo notice:

screen shot 2017-05-22 at 15 17 37

@youknowriad
Copy link
Contributor Author

In WP-admin right now you're moved to the posts list, where you see an Undo notice:

Yeah, I know. We can probably redirect to the posts list, but I'm not sure we can trigger a notice with JS (maybe we have an endpoint I'm not aware of for that?)

@youknowriad
Copy link
Contributor Author

Though I'm getting a 405, rest route not allowed JS error when clicking on it:

This is weird, it's working for me. Maybe the "delete" post endpoint requires an extra capability we should check?

@jasmussen
Copy link
Contributor

Yeah, I know. We can probably redirect to the posts list, but I'm not sure we can trigger a notice with JS (maybe we have an endpoint I'm not aware of for that?)

It'd be nice if we could get this working somehow, or if not we should chat with the #core-restapi folks and add the endpoint to our wishlist.

@youknowriad
Copy link
Contributor Author

I found that we can achieve the required behaviour by redirecting to an url like this /wp-admin/edit.php?post_type=post&trashed=1&ids=19. But to achieve this we need to solve the issue of WP-Admin URL building from JS. I'll create a separate issue for this.

@youknowriad
Copy link
Contributor Author

@jasmussen Could you share more details on the 405? Are you using an admin user? any particular thing about your install? I'm not sure how to reproduce this. WP version?

I would like to get that fixed before merging here and leave the redirection for #867

@jasmussen
Copy link
Contributor

Could you share more details on the 405? Are you using an admin user? any particular thing about your install? I'm not sure how to reproduce this. WP version?

Yes, I'm using the latest version of VVV, and latest WordPress stable. Chrome browser, admin user.

When I click the button nothing happens, but I get a JS error.

Here are some screenshots:

screen shot 2017-05-23 at 11 57 36

screen shot 2017-05-23 at 11 59 59

The REST API URL it appears to be loading, is this: http://local.wordpress.dev/?rest_route=/wp/v2/posts/56

which returns the following information:

{"code":"rest_forbidden","message":"Sorry, you are not allowed to do that.","data":{"status":403}}

Screenshot:

screen shot 2017-05-23 at 12 01 25

Though do note that when I load the same URL inside the dev tools, I instead get an NGINX error.

@youknowriad
Copy link
Contributor Author

Really weird, the only thing I can think of when I look at the code that could trigger that is not having the delete post capability. But I think the admin user has it. Any idea @nylen

@nylen
Copy link
Member

nylen commented May 23, 2017

@youknowriad I think redirecting to the posts listing should go in the initial version, I added this just as a relative link in 36badc0 and we can come up with a better way later on.

@jasmussen I'm not sure what is causing this error for you, especially if you're doing this as an admin user. However, note that visiting that URL directly in your browser is not the same thing:

  • The method is GET rather than DELETE this way
  • There is no nonce being passed, so it's an unauthenticated request

Can you capture the actual response from the DELETE request using the network tools? It should provide more details about the error you're getting.

@nylen
Copy link
Member

nylen commented May 23, 2017

Ah, nevermind, we already have the error info :) It's the 405 Not Allowed error page, note that this is HTML coming from nginx, rather than JSON coming from the REST API.

@jasmussen are you able to save posts correctly? (this uses a request method of PUT). Have you made any changes to the vvv or nginx configs on this installation?

@jasmussen
Copy link
Contributor

@jasmussen are you able to save posts correctly? (this uses a request method of PUT). Have you made any changes to the vvv or nginx configs on this installation?

I have made no changes to the stock vvv install, except the occasional vagrant box update. Posts save fine, except the title disappears on save, but I think that's a known issue.

I want to say this is an issue with my setup, so please don't let this be a blocker for this PR to be merged. Whatever it is, if anything, I'm sure we can fix it in a separate PR. When I get bandwidth, I'll try wp docker instead of vvv, and I'll also try a fresh build of the plugin on my own site. We'll track this down.

@nylen
Copy link
Member

nylen commented May 23, 2017

I agree, this is definitely a server config issue, though I am not sure how that is happening given that you are running stock vvv. We should track this down separately.

You could also try playing with the options under Settings > Permalink, maybe a different setting will enable this to work.

As a potential workaround on our side if this turns out to be a common issue, we can limit the API requests to GET and POST, and send for example ?_method=DELETE. Here's the documentation on the _method parameter.

Copy link
Member

@nylen nylen left a comment

Choose a reason for hiding this comment

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

I think this PR is good to go.

@jasmussen
Copy link
Contributor

I think I figured out at least why I'm getting the "file not found" error. The file doesn't exist:

screen shot 2017-05-24 at 09 25 03

That's the gutenberg/vendor directory right there. According to the path it tries to load, there should be a gutenberg/vendor/plugins/lists/plugin.min.js. But there isn't. I suspect because of the .gitignore.

I don't know if this specific error was the cause of my post trashing woes, but mentioning nonetheless.

@youknowriad
Copy link
Contributor Author

@nylen Thanks James for the redirection. This is good for now. We'll figure out a way to avoid the relative links later.

@jasmussen Have you tried clearing the vendor folder and rebuilding? I don't have this file either but it's not an issue since it's not being loaded in this branch (I think this file is being added in another PR).

@youknowriad youknowriad merged commit eabaad9 into master May 24, 2017
@jasmussen
Copy link
Contributor

Have you tried clearing the vendor folder and rebuilding? I don't have this file either but it's not an issue since it's not being loaded in this branch (I think this file is being added in another PR).

Can you clarify what clearing the vendor folder and rebuilding means?

Also, would this potentially fix my trash button woes, or perhaps just the list block error?

@youknowriad
Copy link
Contributor Author

@jasmussen I don't know if this would fix anything. but something like rm -rf vendor/* && npm run dev

@youknowriad youknowriad deleted the add/trash-post-link branch May 24, 2017 12:22
@nylen
Copy link
Member

nylen commented May 24, 2017

@jasmussen the plugin.min.js thing was a separate issue, it should have been fixed by #863.

@jasmussen
Copy link
Contributor

Well I'll be. Not seeing it in master. So closing #885.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants