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

Fixed image uploading. Both URL and a file #29

Closed
wants to merge 2 commits into from

Conversation

korzol
Copy link

@korzol korzol commented Mar 22, 2019

Hello
Thank you! You did a great job!
iZettle updated their API documentation. At least new there is correct URL to send images to.
Also some small changes like uuid in ImageClient was null.
Also now they require image type even for url, so I had to implement it somehow

Have a good day

@LauLaman
Copy link
Member

LauLaman commented Mar 22, 2019

Great job! Can you also add in the changes in the tests?

This PR should fix #25 image bug

@korzol
Copy link
Author

korzol commented Mar 22, 2019

Will be glad but first I need to learn how it works and where to make changes :)

@korzol
Copy link
Author

korzol commented Mar 24, 2019

Added image upload to unit tests. But testing fails if I run it as is. Complain is:

Fatal error: Trait 'LauLamanApps\IzettleApi\Tests\Unit\MockeryAssertionTrait' not found in ..izettle/vendor/laulamanapps/izettle-api/tests/Unit/GuzzleIzettleClientTest.php on line 24

Checked path and namespaces, even added

require_once 'MockeryAssertionTrait.php'; 

to GuzzleIzettleClientTest.php but seems nothing works.

PHP 7.2.15.
iZettleApi has been installed with composer

Any thoughts ?

@LauLaman
Copy link
Member

LauLaman commented Mar 25, 2019

Hi @korzol
Great job!

Can you tell me how you are running the tests? are you using the Make commands?

I've checkout your pr locally and have come to the following conclusion:

  • LauLamanApps\IzettleApi\Tests\Integration\Client\ImageClientTest:26 We need to enter a real url now. this will fix this tests. i've tried with http://example.com/image.jpg
  • Same goes for LauLamanApps\IzettleApi\Tests\Unit\Client\ImageClientTest:27

If you run with the make command it should work. You can also check if you have mockery installed in your vendor directory.

i've also left you some more feedback. 🤗

self::assertAttributeEquals($imageUrl, 'imageUrl', $productImageUpload);
self::assertSame(['imageUrl' => $imageUrl], json_decode($productImageUpload->getPostBodyData(), true));
// self::assertSame(['imageUrl' => $imageUrl], json_decode($productImageUpload->getPostBodyData(), true));
Copy link
Member

Choose a reason for hiding this comment

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

Please remove dead code

Suggested change
// self::assertSame(['imageUrl' => $imageUrl], json_decode($productImageUpload->getPostBodyData(), true));

self::assertAttributeEquals($imageUrl, 'imageUrl', $productImageUpload);
self::assertSame(['imageUrl' => $imageUrl], json_decode($productImageUpload->getPostBodyData(), true));
// self::assertSame(['imageUrl' => $imageUrl], json_decode($productImageUpload->getPostBodyData(), true));
self::assertSame(['imageFormat' => $imageFormat, 'imageData' => $imageData], json_decode($productImageUpload->getPostBodyData(), true));
Copy link
Member

Choose a reason for hiding this comment

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

This should fix this test

Suggested change
self::assertSame(['imageFormat' => $imageFormat, 'imageData' => $imageData], json_decode($productImageUpload->getPostBodyData(), true));
self::assertSame(['imageFormat' => $imageFormat, 'imageUrl' => $imageUrl], json_decode($productImageUpload->getPostBodyData(), true));

];

$imageUrl = 'https://example.com/image.png';
$imageFormat = $allowedImageType[
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure the code style is inline with the other files:
(alignment equal signs)

run make cs-fix to fix automatically

public function byteArray(string $filename): array
{
$data = file_get_contents($filename);
$array = array();
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure the code style is inline with the other files:
(Short array syntax)

run make cs-fix to fix automatically

}

/**
* Generate byte array similar to the one in C# and Java
Copy link
Member

Choose a reason for hiding this comment

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

Please remove unnecessary comment. It does not add any value.

@@ -31,7 +31,22 @@ public function __construct(string $filename)
{
$this->validateFile($filename);
$this->imageFormat = self::ALLOWED_FILE_TYPES[exif_imagetype($filename)];
Copy link
Member

Choose a reason for hiding this comment

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

Can you add ext-exif to composer.json as a required extension? i've missed this.

@LauLaman LauLaman added the Bug label Mar 25, 2019
@LauLaman LauLaman added this to the Version 1.0 milestone Mar 25, 2019
@LauLaman LauLaman changed the base branch from master to releases/1.0-dev March 26, 2019 21:45
@LauLaman
Copy link
Member

Hi @korzol

Are you able to finish this PR?

@chatlumo
Copy link

Hello @korzol,

Is there any news about this PR ?

@LauLaman
Copy link
Member

LauLaman commented Aug 9, 2019

Closed due to no response. i might fix this myself later on

@LauLaman LauLaman closed this Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants