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

Dependencies API tweaks #1

Merged
merged 31 commits into from
Oct 30, 2020

Conversation

lukaspawlik
Copy link
Collaborator

@spacedmonkey this is still WIP however, it addresses most meaningful feedback found on WordPress#21244

@github-actions
Copy link

github-actions bot commented Oct 23, 2020

Size Change: 0 B

Total Size: 1.21 MB

ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.78 kB 0 B
build/api-fetch/index.js 3.45 kB 0 B
build/autop/index.js 2.84 kB 0 B
build/blob/index.js 665 B 0 B
build/block-directory/index.js 8.72 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 11.1 kB 0 B
build/block-editor/style.css 11.1 kB 0 B
build/block-library/editor-rtl.css 8.97 kB 0 B
build/block-library/editor.css 8.97 kB 0 B
build/block-library/index.js 146 kB 0 B
build/block-library/style-rtl.css 7.83 kB 0 B
build/block-library/style.css 7.84 kB 0 B
build/block-library/theme-rtl.css 837 B 0 B
build/block-library/theme.css 838 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 172 kB 0 B
build/components/style-rtl.css 15.2 kB 0 B
build/components/style.css 15.2 kB 0 B
build/compose/index.js 9.81 kB 0 B
build/core-data/index.js 12.3 kB 0 B
build/data-controls/index.js 772 B 0 B
build/data/index.js 8.77 kB 0 B
build/date/index.js 31.8 kB 0 B
build/deprecated/index.js 768 B 0 B
build/dom-ready/index.js 571 B 0 B
build/dom/index.js 4.46 kB 0 B
build/edit-navigation/index.js 11.2 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.4 kB 0 B
build/edit-post/style.css 6.39 kB 0 B
build/edit-site/index.js 22.1 kB 0 B
build/edit-site/style-rtl.css 3.88 kB 0 B
build/edit-site/style.css 3.88 kB 0 B
build/edit-widgets/index.js 26.4 kB 0 B
build/edit-widgets/style-rtl.css 3.12 kB 0 B
build/edit-widgets/style.css 3.12 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 43.1 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.85 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 735 B 0 B
build/format-library/index.js 7.7 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 623 B 0 B
build/i18n/index.js 3.57 kB 0 B
build/is-shallow-equal/index.js 712 B 0 B
build/keyboard-shortcuts/index.js 2.52 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.11 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.34 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.42 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.43 kB 0 B
build/priority-queue/index.js 791 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13.2 kB 0 B
build/server-side-render/index.js 2.77 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.22 kB 0 B

compressed-size-action

@lukaspawlik lukaspawlik marked this pull request as ready for review October 23, 2020 15:27
@lukaspawlik
Copy link
Collaborator Author

@spacedmonkey I think some of these tests fail as they are executed out of main repo - All PHP stuff tests pass on my local without any issues.

lib/rest-api.php Outdated Show resolved Hide resolved
'role' => 'author',
)
);
self::$subscriber_id = $factory->user->create(
Copy link
Owner

Choose a reason for hiding this comment

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

We should use this user to test auth. Use a get_item / get_items call as a subscriber

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This has been added. Subscriber cannot view them according to tests.

'role' => 'administrator',
)
);
self::$editor_id = $factory->user->create(
Copy link
Owner

Choose a reason for hiding this comment

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

What is this used for ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's been used for tests. Unfortunately editors can't read scripts/styles. The same applies to authors.

'role' => 'editor',
)
);
self::$author_id = $factory->user->create(
Copy link
Owner

Choose a reason for hiding this comment

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

Can we test to see you can see block assets and their dependencies as an author.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's been used for tests. Unfortunately authors can't read scripts/styles. I think this should be addressed right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So tests are executed against not core assets and mentioned roles (authors, editors) can't see it. That's not a case for core assets. Shall we address this to ensure that authors, editors can also read scripts / styles? That doesn't seem to be risky at all.

$this->assertEquals( home_url( '/test.css' ), $data['src'] );
$this->assertEquals( home_url( '/test.css' ), $data['url'] );
}

Copy link
Owner

Choose a reason for hiding this comment

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

Add tests for no user / subscriber / author to see that permissions checks work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

* Test single block script.
*/
public function test_get_item_block_script() {
foreach ( array( 0, self::$subscriber_id, self::$editor_id, self::$author_id, self::$admin_id, self::$superadmin_id ) as $user_id ) {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't personally like this style of loop. Can we just use a dataprovider here and other places that this format is used? Using dataprovider really helps in debugging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@spacedmonkey I tried to use data providers and they were not delivering the right data - if they return primitives like integers then they work fine. When I tried to return class props they were always set to null. Any idea what I did incorrectly?

@spacedmonkey
Copy link
Owner

Barring this fix https://github.com/spacedmonkey/gutenberg/pull/1/files#r514452318, I think we are ready to merge @lukaspawlik 🎉

* Test single style.
*/
public function test_get_item() {
foreach ( array( self::$admin_id, self::$superadmin_id ) as $user_id ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

* Test multiple styles.
*/
public function test_get_items() {
foreach ( array( self::$admin_id, self::$superadmin_id ) as $user_id ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Same here.

Comment on lines 105 to 109
self::delete_user( self::$superadmin_id );
self::delete_user( self::$admin_id );
self::delete_user( self::$editor_id );
self::delete_user( self::$author_id );
self::delete_user( self::$subscriber_id );
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
self::delete_user( self::$superadmin_id );
self::delete_user( self::$admin_id );
self::delete_user( self::$editor_id );
self::delete_user( self::$author_id );
self::delete_user( self::$subscriber_id );
foreach( self::$users_map as $key => $user_id ){
self::delete_user( $user_id );
}

/**
* @var int
*/
protected static $admin_id;
Copy link
Owner

Choose a reason for hiding this comment

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

We can get rid of these class properties as well.

@lukaspawlik
Copy link
Collaborator Author

@spacedmonkey ready for your review (another one 😄 )

Copy link
Owner

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

👍 Looking good.

@spacedmonkey spacedmonkey merged commit 65b2d87 into feature/dependancy-api Oct 30, 2020
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.

2 participants