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

Block Directory: Fix installing blocks #23096

Merged

Conversation

TimothyBJacobs
Copy link
Member

Description

I broke block directory inserting on master after #22454 was committed. This should fix those errors.

A block installation error is now expressed by returning an error response,
this means looking for a success property no longer works since we are
returning the installed plugin information instead.

Also switches to making an OPTIONS request to the search controller instead
of making an "empty" search request as a search term is now required.

How has this been tested?

Tested manually by opening the block directory inserter, searching for "peculiar" and installing the returned block.

Types of changes

Bug Fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

A block installation error is now expressed by returning an error response,
this means looking for a success property no longer works since we are
returning the installed plugin information instead.

Also switches to making an OPTIONS request to the search controller instead
of making an "empty" search request as a search term is now required.
@TimothyBJacobs TimothyBJacobs added REST API Interaction Related to REST API [Feature] Block Directory Related to the Block Directory, a repository of block plugins labels Jun 11, 2020
@TimothyBJacobs TimothyBJacobs mentioned this pull request Jun 11, 2020
6 tasks
@TimothyBJacobs
Copy link
Member Author

I was able to fix the unit test error, but I'm struggling to figure out how to fix the end to end test failure. AFAICT, it's breaking here:

// Grab the first block in the list -> Needs to be the first one, the mock response expects it.
const addBtn = await page.waitForSelector(
	'.block-directory-downloadable-blocks-list li:first-child button'
);

I think the search is working. But I don't know how to debug further.

@ryelle
Copy link
Contributor

ryelle commented Jun 12, 2020

If you're running it locally, you can use --puppeteer-interactive to watch the test. It looks like the permissions request isn't mocked, so it's failing that check & displays the "Please contact your site administrator to install new blocks." message.

@TimothyBJacobs
Copy link
Member Author

Aha! Thanks for the tip @ryelle! Working on a fix.

@TimothyBJacobs
Copy link
Member Author

Looks like that fixed it, I'm not sure if I went about it the correct way. But I didn't see other examples of mocking headers, and didn't want to touch the e2e-test-utils package. The PHPUnit tests are failing for an unrelated reason.

@TimothyBJacobs
Copy link
Member Author

Test suite is now passing.

Copy link
Contributor

@StevenDufresne StevenDufresne left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! :)

if ( error.code === 'rest_user_cannot_view' ) {
yield setInstallBlocksPermission( false );

let allowHeader;
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 it makes sense to have this here until we are out of experimental status and move to:

export function* canUser( action, resource, id ) {

@StevenDufresne
Copy link
Contributor

Test suite is now passing.

Running it interactively seems to work. There is a warning being thrown failing my tests locally but I don't think its related to the block directory.

["Warning: React does not recognize the `%s` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `%s` instead. If you accidentally passed it from a parent component, remove it from the DOM element.%s isSelected isselected·
        in button (created by ForwardRef)
        in h (created by ForwardRef)
        in ForwardRef
        in Unknown (created by Context.Consumer)
        in WithPluginContext(Component)
        in div (created by r)
        in r (created by Context.Consumer)
        in v (created by S)
        in S
        in Unknown (created by ar)
        in div (created by ar)
        in div (created by ar)
        in ar (created by dn)
        in div
        in div
        in Unknown (created by NavigateRegions(Component))
        in div (created by NavigateRegions(Component))
        in NavigateRegions(Component) (created by dn)
        in div (created by h)
        in h (created by dn)
        in dn (created by n)
        in r (created by n)
        in Unknown
        in Unknown (created by Context.Consumer)
        in WithRegistryProvider(Component) (created by r)
        in Hn (created by r)
        in Gt (created by r)
        in Gt (created by r)
        in r (created by WithDispatch(r))
        in WithDispatch(r)
        in Unknown (created by WithSelect(WithDispatch(r)))
        in WithSelect(WithDispatch(r))
        in Unknown (created by Context.Consumer)
        in WithRegistryProvider(WithSelect(WithDispatch(r))) (created by n)
        in div (created by r)
        in r (created by n)
        in k (created by r)
        in r (created by n)
        in StrictMode (created by n)
        in n (created by WithDispatch(n))
        in WithDispatch(n)
        in Unknown (created by WithSelect(WithDispatch(n)))
        in WithSelect(WithDispatch(n))"],["Warning: React does not recognize the `%s` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `%s` instead. If you accidentally passed it from a parent component, remove it from the DOM element.%s isSelected isselected·
        in button (created by ForwardRef)
        in h (created by ForwardRef)
        in ForwardRef
        in Unknown (created by Context.Consumer)
        in WithPluginContext(Component)
        in div (created by r)
        in r (created by Context.Consumer)
        in v (created by S)
        in S
        in Unknown (created by ar)
        in div (created by ar)
        in div (created by ar)
        in ar (created by dn)
        in div
        in div
        in Unknown (created by NavigateRegions(Component))
        in div (created by NavigateRegions(Component))
        in NavigateRegions(Component) (created by dn)
        in div (created by h)
        in h (created by dn)
        in dn (created by n)
        in r (created by n)
        in Unknown
        in Unknown (created by Context.Consumer)
        in WithRegistryProvider(Component) (created by r)
        in Hn (created by r)
        in Gt (created by r)
        in Gt (created by r)
        in r (created by WithDispatch(r))
        in WithDispatch(r)
        in Unknown (created by WithSelect(WithDispatch(r)))
        in WithSelect(WithDispatch(r))
        in Unknown (created by Context.Consumer)
        in WithRegistryProvider(WithSelect(WithDispatch(r))) (created by n)
        in div (created by r)
        in r (created by n)
        in k (created by r)
        in r (created by n)
        in StrictMode (created by n)
        in n (created by WithDispatch(n))
        in WithDispatch(n)
        in Unknown (created by WithSelect(WithDispatch(n)))
        in WithSelect(WithDispatch(n))"]


@TimothyBJacobs
Copy link
Member Author

I got that error message as well.

Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

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

Looks good & is working correctly now. FWIW I'm seeing the " React does not recognize the %s prop on a DOM element" console error on master too, so it's not an issue from this PR.

@TimothyBJacobs TimothyBJacobs merged commit 9124907 into WordPress:master Jun 15, 2020
@github-actions github-actions bot added this to the Gutenberg 8.4 milestone Jun 15, 2020
This was referenced Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block Directory Related to the Block Directory, a repository of block plugins REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants