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

Reusable blocks: Use view context, always include title.raw and content.raw #12084

Merged
merged 8 commits into from Nov 20, 2018

Conversation

Projects
None yet
3 participants
@noisysocks
Member

noisysocks commented Nov 20, 2018

Supersedes #11891. Fixes #11343.

Existing reusable blocks on a post would not render if the user was an author or a contributor. This happened because requests to fetch a single block or post are blocked when ?context=edit is passed and the current user is not an editor.

The fix is to stop passing ?context=edit to block requests, but this causes another problem which is that Gutenberg needs access to both title.raw and content.raw in the API response.

We therefore both stop passing ?context=edit to block requests and modify the blocks API so that title.raw and content.raw are always provided.

How to test

  1. Create two users, user1 (any role) and user2 (author role)
  2. Log in as user1 and create a reusable block
  3. Log in as user2 and create a post using user1's block
  4. The block should render correctly.
  5. Save the post and refresh the edit screen.
  6. The block should again render correctly.
@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 20, 2018

Wasn't able to re-open #11891 so this is a new PR to replace it.

I updated the inline comment to explain how unset() achieves what we want, and have tweaked the Reusable Block's description in the sidebar with an aim to better explain that blocks can be seen and used by non-editors:

screen shot 2018-11-20 at 11 45 22

cc. @mtias @pento @peterwilsoncc

@pento

Given that we should really only ever need the raw post data, and not the rendered post data, I'm inclined to think that we should remove rendered from the returned data. This will help to avoid the confusion from an API perspective.

Show resolved Hide resolved lib/class-wp-rest-blocks-controller.php Outdated
Show resolved Hide resolved phpunit/class-rest-blocks-controller-test.php Outdated
Reusable blocks: Use view context
Existing reusable blocks on a post would not render if the user was an
author or a contributor. This happened because requests to fetch a
single block or post are blocked when ?context=edit is passed and the
current user is not an editor.

The fix is to stop passing `?context=edit` to block requests, but this
causes another problem which is that Gutenberg needs access to both
`title.raw` and `content.raw` in the API response.

We therefore both stop passing `?context=edit` to block requests *and*
modify the blocks API so that `title.raw` and `content.raw` are always
provided.

Lastly, we modify the blocks API to always omit `content.rendered`, as
it doesn't make sense for a reusable block to have rendered content on
its own, since rendering a block requires it to be inside a post or a
page.

@noisysocks noisysocks force-pushed the fix/reusable-blocks-context branch from b965514 to 537c3ad Nov 20, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 20, 2018

Given that we should really only ever need the raw post data, and not the rendered post data, I'm inclined to think that we should remove rendered from the returned data. This will help to avoid the confusion from an API perspective.

I've made this change and added some comments explaining the rationale.

Typo: /** → /*
Co-Authored-By: noisysocks <robert@noisysocks.com>
@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 20, 2018

I removed title.rendered in 4689596. It's not very relevant in the case of blocks since we don't ever render a block by itself on the front-end.

Omit `title.rendered` from the blocks REST API
The rendered title isn't used, so we may as well remove it so that
`content` and `title` are aligned.

@noisysocks noisysocks force-pushed the fix/reusable-blocks-context branch from 55c7800 to 4689596 Nov 20, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 20, 2018

What about contributor role? I did the following:

  1. Create a post as a contributor.
  2. Added a paragraph.
  3. Saved as a draft.
  4. Switched to the admin role.
  5. Turned the existing paragraph into a reusable block.
  6. Saved updates.
  7. Logged into the contributor account.
  8. I still can edit my post, but reusable blocks don't load at all.

screen shot 2018-11-20 at 08 03 16

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 20, 2018

I can confirm it works perfectly fine for the editor role.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 20, 2018

As a editor I can't edit reusable block created by a user with admin role. Should we detect it and prevent changes in the UI or rather relax this check?

screen shot 2018-11-20 at 08 21 08

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 20, 2018

Thanks for the eyes, @gziolo!

Contributors aren't allowed to create or edit reusable blocks. It seems that step 1 of your instructions is silently failing and inserting an invalid <!wp:block --/> block 😢

It's important that we get the PHP changes in this PR in sooner than later so I think we should address the bug you've found as a separate issue.

There's a few UI issues surrounding reusable blocks and permissions. I'll try to address as many as I can tonight and tomorrow.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 20, 2018

<!-- wp:block {"ref":47} /-->

<!-- wp:block /-->

I turned paragraph block into a reusable one when using admin role. It looks like you can't load existing reusable blocks as a contributor.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 20, 2018

I added a fix for the contributor role in b0333f3.

I have no idea whether it is valid or not. @youknowriad I would appreciate your feedback.

@gziolo gziolo force-pushed the fix/reusable-blocks-context branch from b0333f3 to a15b385 Nov 20, 2018

@gziolo

gziolo approved these changes Nov 20, 2018

I added the last commit to fix the issue for the contributor role. I would appreciate a sanity check from someone else and we can 🚢

Let's work on UI improvements in the follow-up PRs.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Nov 20, 2018

Thanks for your help @gziolo! Some unit tests needed to be updated—I've fixed that up in 5d27397.

@gziolo gziolo merged commit b462725 into master Nov 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the fix/reusable-blocks-context branch Nov 20, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Reusable blocks: Use view context, always include title.raw and conte…
…nt.raw (WordPress#12084)

* Reusable blocks: Use view context

Existing reusable blocks on a post would not render if the user was an
author or a contributor. This happened because requests to fetch a
single block or post are blocked when ?context=edit is passed and the
current user is not an editor.

The fix is to stop passing `?context=edit` to block requests, but this
causes another problem which is that Gutenberg needs access to both
`title.raw` and `content.raw` in the API response.

We therefore both stop passing `?context=edit` to block requests *and*
modify the blocks API so that `title.raw` and `content.raw` are always
provided.

Lastly, we modify the blocks API to always omit `content.rendered`, as
it doesn't make sense for a reusable block to have rendered content on
its own, since rendering a block requires it to be inside a post or a
page.

* Delete fake user after each test case, not after all test cases

* Typo: /** → /*

Co-Authored-By: noisysocks <robert@noisysocks.com>

* Create all fake data before any test runs

* Omit `title.rendered` from the blocks REST API

The rendered title isn't used, so we may as well remove it so that
`content` and `title` are aligned.

* Fix fetching when using reusable blocks as a contributor

* Fix reusable block effect tests

* Fix reusable block E2E tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment