Feature/#17 Adding mediaItem mutations. #189

hughdevore · 2017-08-04T20:05:26Z

No description provided.

jasonbahl · 2017-08-04T20:06:32Z

@hughdevore there's a merge conflict?

hughdevore · 2017-08-04T20:07:48Z

@jasonbahl, the conflicts were in the test file but there weren't actually any conflicts. You can check the last commit where I resolved it in browser.

coveralls · 2017-08-04T20:16:16Z

Coverage decreased (-0.6%) to 92.581% when pulling ee7ab0e on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

jasonbahl · 2017-08-08T16:03:07Z

src/Type/MediaItem/MediaItemCreate.php

+				/**
+				 * Stop now if a user isn't allowed to create a mediaItem
+				 */
+				if ( ! current_user_can( $post_type_object->cap->create_posts ) ) {


So, there's a specific upload_files permission we should check for instead of this.

See how the REST API is doing it here:
https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php#L72-L74

jasonbahl · 2017-08-08T16:07:14Z

src/Type/MediaItem/MediaItemCreate.php

+					throw new \Exception( __( 'Sorry, you are not allowed to create mediaItems', 'wp-graphql' ) );
+				}
+
+				/**


We should check to see if the user has permission to edit the related post (parentId).

Here's how the REST API does that (their input is "post" and ours is "parentId")

https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php#L76-L84

jasonbahl · 2017-08-08T16:54:30Z

src/Type/MediaItem/MediaItemCreate.php

+				 * If the mediaItem being created is being assigned to another user that's not the current user, make sure
+				 * the current user has permission to edit others mediaItems
+				 */
+				if ( ! empty( $input['authorId'] ) && get_current_user_id() !== $input['authorId'] && ! current_user_can( $post_type_object->cap->edit_others_posts ) ) {


So, the REST API actually doesn't do this check for setting Authors on mediaItems. . .I'm not against keeping this, but I'm curious why they don't have a check like this. Might be worth looking into to make sure this makes sense here.

Yeah, I'd say we keep it. I'm kind of surprised they wouldn't have this check if they're allowing a user to create attachments under another user's ID. ¯_(ツ)_/¯

jasonbahl · 2017-08-08T16:59:40Z

src/Type/MediaItem/MediaItemDelete.php

+			'inputFields'         => [
+				'id'          => [
+					'type'        => Types::non_null( Types::id() ),
+					// translators: The placeholder is the name of the post's post_type being deleted


don't need this translation note anymore

jasonbahl · 2017-08-08T17:00:12Z

src/Type/MediaItem/MediaItemDelete.php

+
+		self::$mutation['mediaItem'] = Relay::mutationWithClientMutationId( [
+			'name'                => esc_html( $mutation_name ),
+			// translators: The placeholder is the name of the object type


don't need this translation note anymore.

jasonbahl · 2017-08-08T21:12:13Z

src/Type/MediaItem/MediaItemDelete.php

+				/**
+				 * Delete the mediaItem
+				 */
+				$deleted = wp_delete_attachment( $id_parts['id'], $force_delete );


Since this can return false on failure, we should account for that and throw an exception

jasonbahl · 2017-08-08T21:14:36Z

src/Type/MediaItem/MediaItemMutation.php

+
+		if ( ! empty( $file['type'] ) ) {
+			$insert_post_args['post_mime_type'] = $file['type'];
+		} else if ( ! empty( $input['fileType'] ) ) {


jasonbahl · 2017-08-08T21:14:56Z

src/Type/MediaItem/MediaItemMutation.php

+		}
+
+		if ( ! empty( $input['dateGmt'] ) && false !== strtotime( $input['dateGmt'] ) ) {
+			$insert_post_args['post_date_gmt'] = date("Y-m-d H:i:s", strtotime( $input['dateGmt'] ) );


jasonbahl · 2017-08-08T21:15:01Z

src/Type/MediaItem/MediaItemMutation.php

+		 * NOTE: These are organized in the same order as: http://v2.wp-api.org/reference/media/#schema-meta
+		 */
+		if ( ! empty( $input['date'] ) && false !== strtotime( $input['date'] ) ) {
+			$insert_post_args['post_date'] = date("Y-m-d H:i:s", strtotime( $input['date'] ) );


space & single quotes instead of double

jasonbahl · 2017-08-08T21:15:43Z

src/Type/MediaItem/MediaItemMutation.php

+
+		if ( ! empty( $input['title'] ) ) {
+			$insert_post_args['post_title'] = $input['title'];
+		} else if ( ! empty( $file['file'] ) ) {


jasonbahl · 2017-08-08T21:21:00Z

src/Type/MediaItem/MediaItemCreate.php

+				/**
+				 * If the mediaItem file is from a local server, use wp_upload_bits before saving it to the uploads folder
+				 */
+				if ( 'false' === filter_var( $input['filePath'], FILTER_VALIDATE_URL ) ) {


Should not be 'false' but just false without the single quotes.

jasonbahl · 2017-08-08T21:24:21Z

@hughdevore overall this looks pretty good. All the comments are relatively minor.

Then, good start on tests, but need more to get "fully covered". You can see the coverage reports here:

…ithub.com/hughdevore/wp-graphql into feature/wp-graphql#17-mediaItem-mutations

hughdevore · 2017-08-09T17:21:40Z

Thanks @jasonbahl! I'll start work on increasing the test coverage

coveralls · 2017-08-09T17:27:00Z

Coverage decreased (-0.8%) to 92.358% when pulling dfed277 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-09T17:27:00Z

Coverage decreased (-0.8%) to 92.358% when pulling dfed277 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-11T15:37:04Z

Coverage increased (+0.6%) to 93.813% when pulling f48b55d on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

hughdevore · 2017-08-11T16:11:09Z

@jasonbahl, finally increased coverage! But.. my builds are failing for older versions of php.

jasonbahl · 2017-08-11T17:31:26Z

@hughdevore fix it!

coveralls · 2017-08-11T21:48:29Z

Coverage increased (+0.6%) to 93.813% when pulling 6a1fd48 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-11T22:15:14Z

Coverage decreased (-0.3%) to 92.895% when pulling 2cf1b04 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-11T23:39:37Z

Coverage decreased (-0.3%) to 92.895% when pulling 6c84219 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-11T23:56:54Z

Coverage decreased (-0.3%) to 92.895% when pulling e4c1471 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-11T23:56:54Z

Coverage decreased (-0.3%) to 92.895% when pulling e4c1471 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-14T21:21:04Z

Coverage decreased (-0.08%) to 93.096% when pulling 598668c on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-14T22:40:25Z

Coverage decreased (-0.06%) to 93.124% when pulling 424dc47 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-14T23:02:43Z

Coverage decreased (-0.03%) to 93.15% when pulling e703053 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-14T23:41:21Z

Coverage decreased (-0.03%) to 93.148% when pulling 7e4aba7 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-14T23:55:06Z

Coverage increased (+0.01%) to 93.193% when pulling fbba5fb on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

jasonbahl · 2017-08-15T15:01:39Z

@hughdevore good work on this. Checking it out now and will let you know if I have any further feedback.

Thanks!

coveralls · 2017-08-15T15:57:54Z

Coverage increased (+0.06%) to 93.241% when pulling 6110808 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

jasonbahl · 2017-08-15T16:06:48Z

tests/test-media-item-mutations.php

+		/**
+		 * download_url_file will equal true if the file is already included
+		 */
+		$download_url_file = require_once( ABSPATH . 'wp-admin/includes/file.php' );


so, I would actually test running a mutation where this file was already included (like if another plugin included it already) to make sure that the duplicate includes don't cause an issue.

So, something like:

require_once( ABSPATH . 'wp-admin/includes/file.php' ); $mutation = 'define your mutation here'; $variables = [ 'Define your variables here' ]; $actual = do_graphql_request( $mutation, 'createMediaItemWithDuplicateFileInclude', $variables ); // Then make your assertion(s) that the mutation processed without error. . .

If there was an error due to the functions in that file being defined multiple times or whatever, we'll find out here.

But right now the tests are only testing whether PHP's require_once actually does require the file properly, which is fine, but we want to also make sure that requiring that file when it's already been required by another part of the code doesn't cause issues.

coveralls · 2017-08-15T16:07:17Z

Coverage increased (+0.06%) to 93.241% when pulling 6110808 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

jasonbahl · 2017-08-15T16:10:10Z

tests/test-media-item-mutations.php

+	 * @access public
+	 * @return void
+	 */
+	public function testUmiInvalidId() {


let's update the test names to be more explicit.

testUmi doesn't mean anything to me if I'm working on an unrelated feature and see that my code broke testUmi. . .but if I see that I broke testUpdateMediaItemMutationInvalidID then I have more clarity and don't have to waste time trying to find out what Umi stands for

same with Cmi and Dmi. . .let's be explicit and use CreateMediaItem and DeleteMediaItem

A quick search/replace should work 🤞

jasonbahl · 2017-08-15T16:12:22Z

src/Type/MediaItem/Mutation/MediaItemCreate.php

+				/**
+				 * Insert the mediaItem and retrieve it's data
+				 */
+				$file = wp_handle_sideload( $file_data, $overrides );


not a big deal, but I'd prefer a line break between this and the next code comment block

jasonbahl · 2017-08-15T16:13:12Z

src/Type/MediaItem/Mutation/MediaItemCreate.php

+				/**
+				 * Stop now if a user isn't allowed to edit the parent post
+				 */
+				// Attaching media to a post requires ability to edit said post.


the mixed use of comment styles here is weird, and the second one is maybe slightly misplaced?

jasonbahl · 2017-08-15T16:13:18Z

src/Type/MediaItem/Mutation/MediaItemCreate.php

+				/**
+				 * Insert the mediaItem
+				 */
+				$attachment_id = wp_insert_attachment( $media_item_args, $file['file'], $attachment_parent_id );


jasonbahl · 2017-08-15T16:13:27Z

src/Type/MediaItem/Mutation/MediaItemCreate.php

+				 * Generate and update the mediaItem's metadata
+				 */
+				$attachment_data = wp_generate_attachment_metadata( $attachment_id, $file['file'] );
+				$attachment_data_update = wp_update_attachment_metadata( $attachment_id, $attachment_data );


jasonbahl · 2017-08-15T16:13:47Z

src/Type/MediaItem/Mutation/MediaItemDelete.php

+				/**
+				 * Delete the mediaItem
+				 */
+				$deleted = wp_delete_attachment( $id_parts['id'], $force_delete );


jasonbahl

@hughdevore this looks pretty good. Some minor formatting requests, then some suggestions on the tests.

But I think this is about mergeable!

Good stuff.

coveralls · 2017-08-15T16:15:38Z

Coverage increased (+0.06%) to 93.241% when pulling 6110808 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

coveralls · 2017-08-15T16:46:46Z

Coverage increased (+0.06%) to 93.241% when pulling 8351a52 on hughdevore:feature/#17-mediaItem-mutations into a434aa2 on wp-graphql:develop.

hughdevore · 2017-08-15T16:50:39Z

@jasonbahl, we should be all set to merge here. Let me know if you have anything else. Thanks!

hughdevore added 4 commits August 4, 2017 13:52

wp-graphql#17 - Adding new enum for media item status.

9fc2256

wp-graphql#17 - Fixing mediaItem mime type field.

51e81be

wp-graphql#17 - Adding mediaItem mutations.

06a6cfa

wp-graphql#17 - Adding mediaItem mutation tests.

c717e11

Merge branch 'develop' into feature/wp-graphql#17-mediaItem-mutations

ee7ab0e

jasonbahl requested changes Aug 8, 2017

View reviewed changes

jasonbahl reviewed Aug 8, 2017

View reviewed changes

hughdevore added 2 commits August 9, 2017 11:17

wp-graphql#17 Adding code review changes.

4a4f57f

Merge branch 'feature/wp-graphql#17-mediaItem-mutations' of https://g…

dfed277

…ithub.com/hughdevore/wp-graphql into feature/wp-graphql#17-mediaItem-mutations

hughdevore added 2 commits August 10, 2017 19:06

wp-graphql#17 Updating unit tests.

c03dbff

Updating code for testing.

f48b55d

wp-graphql#17 Fixing php55 fatal error.

6a1fd48

wp-graphql#17 Fixing php55 fatal error.

2cf1b04

wp-graphql#17 Finalizing unit tests.

6c84219

wp-graphql#17 Finalizing unit tests.

e4c1471

wp-graphql#17 Adding unit tests and code cleanup.

424dc47

wp-graphql#17 Adding unit tests and code cleanup.

e703053

wp-graphql#17 Adding unit tests and code cleanup.

7e4aba7

wp-graphql#17 Adding unit tests and code cleanup.

fbba5fb

hughdevore added 3 commits August 15, 2017 09:46

wp-graphql#17 Adding unit tests and code cleanup.

989c1de

wp-graphql#17 Adding unit tests and code cleanup.

c08d852

wp-graphql#17 Adding unit tests and code cleanup.

6110808

jasonbahl reviewed Aug 15, 2017

View reviewed changes

jasonbahl requested changes Aug 15, 2017

View reviewed changes

wp-graphql#17 Adding final code review changes.

8351a52

jasonbahl approved these changes Aug 15, 2017

View reviewed changes

jasonbahl merged commit b9eadc3 into wp-graphql:develop Aug 15, 2017

jasonbahl mentioned this pull request Aug 15, 2017

Attachment Mutations #17

Closed

Feature/#17 Adding mediaItem mutations. #189

Feature/#17 Adding mediaItem mutations. #189

Conversation

hughdevore commented Aug 4, 2017

jasonbahl commented Aug 4, 2017

hughdevore commented Aug 4, 2017

coveralls commented Aug 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonbahl commented Aug 8, 2017

hughdevore commented Aug 9, 2017

coveralls commented Aug 9, 2017 • edited

coveralls commented Aug 9, 2017 • edited

coveralls commented Aug 11, 2017 • edited

hughdevore commented Aug 11, 2017

jasonbahl commented Aug 11, 2017

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017

coveralls commented Aug 14, 2017 • edited

coveralls commented Aug 14, 2017 • edited

coveralls commented Aug 14, 2017 • edited

coveralls commented Aug 14, 2017 • edited

coveralls commented Aug 14, 2017 • edited

jasonbahl commented Aug 15, 2017

coveralls commented Aug 15, 2017 • edited

Choose a reason for hiding this comment

coveralls commented Aug 15, 2017 • edited

jasonbahl Aug 15, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonbahl left a comment

Choose a reason for hiding this comment

coveralls commented Aug 15, 2017 • edited

coveralls commented Aug 15, 2017 • edited

hughdevore commented Aug 15, 2017

coveralls commented Aug 4, 2017 •

edited

coveralls commented Aug 9, 2017 •

edited

coveralls commented Aug 9, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited

coveralls commented Aug 15, 2017 •

edited

coveralls commented Aug 15, 2017 •

edited

jasonbahl Aug 15, 2017 •

edited

coveralls commented Aug 15, 2017 •

edited

coveralls commented Aug 15, 2017 •

edited