API uploads images via URL

[calebhaye]

Description

This enhancement give the images API the ability to create images from URLs. A
simple check determines whether or not the attachment parameter is a URL. If it
is a URL, OpenURI is used to download the image and it subsequently undergoes
the standard post upload processing (via paperclip). If it is not a URL, then
the attachment parameter is handled in the same manner it was prior to this
commit.

Fixes #3572

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[kennyadsl]

@calebhaye I think that HoundCI is complaining at valid things, can you please take a look?

[calebhaye]

Yes, I’ve been meaning to.  I’ll post some fixups soon.

…

On Apr 3, 2020, at 4:01 AM, Alberto Vena ***@***.***> wrote: @calebhaye I think that HoundCI is complaining at valid things, can you please take a look? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or unsubscribe .<embedded image>

[calebhaye]

Okay, all ready :) @kennyadsl

[kennyadsl]

Thanks a lot!

I left a couple of notes, can we also add some documentation in this API endpoint description?

[kennyadsl]

Why do we need to check the host here?

[calebhaye]

Maybe we don't? Will fix.

[calebhaye]

fixed in latest commit.

[kennyadsl]

What about using an image that belongs to Solidus, e.g. https://github.com/solidusio/brand/raw/1827e7afb7ebcf5a1fc9cf7bf6cf9d277183ef11/PNG/solidus-logo-dark.png ?

This way the test suite is safe, whatever Google decides to do with their assets.

[calebhaye]

I like that suggestion. I was definitely unsure about this one. Will fix

[calebhaye]

fixed in latest commit.

[calebhaye]

Thanks a lot!

I left a couple of notes, can we also add some documentation in this API endpoint description?

Hi @kennyadsl,

The notes you left have been addressed.

The current implementation has zilch for documentation in the API endpoint description, which is probably why neither of us know if it really works or not. I could add something for docs, but I honestly don't know where/how to do that. Will you please point me in the right direction? I'd like to wrap this one up soon if we can 👍, and at present it seems like the only blocker for merging is your request for documentation.

Thank you

[kennyadsl]

I'm figuring out the best place where to add that info to the documentation, in the meantime, I've probably found a small issue with the code. Let me know if it's a valid concern, thanks again!

[kennyadsl]

Looking twice, is this removing the other attributes in image_params and creating an image with only the attachment? I think we should update the image_params instead, preserving the rest of the attributes.

[calebhaye]

I think it may be. I'll do some debugging soon and get back to you.

[calebhaye]

@kennyadsl Good catch. I added a fix that addresses your concern. The line following the line you commented on now updates the resultant Spree::Image object with the other attributes within image_params, except for :attachment

I tried updating image_params instead, as per your suggestion, but that resulted in the following error when running the spec:

Paperclip::AdapterRegistry::NoHandlerError:
       No handler found for "https://github.com/solidusio/brand/raw/1827e7afb7ebcf5a1fc9cf7bf6cf9d277183ef11/PNG/solidus-logo-dark.png"

So, I opted to create the image first using image_params[:attachment], and then update all the other attributes using ActionController::Parameters except method.

I also updated the spec so that it fails without the newly added line.

[kennyadsl]

It sounds weird... I'll try to take a look soon and see if I find why this happens. I'd prefer if we can make a single INSERT query here, instead of also updating the record later.

[kennyadsl]

I'm trying locally and I got what you mean now. It looks like a bug in paperclip: if it doesn't have the adapter with create and all the rest of attributes I can't get why it has the adapter if you create the record only with the attachment one...

Anyway, I think we can avoid the double query with this code:

# frozen_string_literal: true

module Spree
  module Api
    class ImagesController < Spree::Api::BaseController
      def create
        authorize! :create, Image

        @image = scope.images.build(image_params.except(:attachment))
        @image.attachment = prepared_attachment
        @image.save

        respond_with(@image, status: 201, default_template: :show)
      end

      private

      def prepared_attachment
        uri = URI.parse image_params[:attachment]

        if uri.is_a? URI::HTTP
          URI.open(image_params[:attachment])
        else
          image_params[:attachment]
        end
      rescue URI::InvalidURIError
        image_params[:attachment]
      end
    end
  end
end

I'm also wondering if we need something similar in the update method but that could be part of another PR if you don't have time and need for that to work in your use case.

[aldesantis]

@calebhaye I was wondering if you had a chance to look at this? Would be really cool to get this merged. 🙂

[calebhaye]

@aldesantis yep! Just pushed the latest, which includes an updated update method and a new spec for testing the update method when updating the attachment property.

@kennyadsl please note: I originally updated the update method to look like this:

@image.assign_attributes(image_params.except(:attachment))
@image.attachment = prepared_attachment if image_params[:attachment].present?
@image.save

But I changed it to the following because I feel as though it reads more logically:

if image_params[:attachment].present?
  @image.assign_attributes(image_params.except(:attachment))
  @image.attachment = prepared_attachment
  @image.save
else
  @image.update image_params
end

I'd rather have it make more sense to developers who reference the code in the future, than to have fewer lines

[kennyadsl]

For the documentation, I mean adding a small description here, something like:

      attachment:
        type: string
        description: |
          The URL of the image to attach.

Also, can you please squash commits into a single one?

Thanks again.

[calebhaye]

For the documentation, I mean adding a small description here, something like:

      attachment:
        type: string
        description: |
          The URL of the image to attach.

Also, can you please squash commits into a single one?

Thanks again.

Thanks for the pointer. If that's the prescribed methodology, I'll add the docs there. I noticed that the other API controllers (like the create method in the ProductsController, for instance), just have code comments and no actual docs that manifest on Stoplight. I'll leave it up to you, should I add the docs as a comment, add them to the aforementioned reference point suggested by you, or both? Please advise @kennyadsl

And yes of course I'll squash the commits -- thx

[calebhaye]

Should be good to go 🤞

[kennyadsl]

Thank you!