Image upload in creating issue #6962

erbunao · 2014-05-13T08:17:34Z

[WIP] Expecting for feedback. We have already implemented:

Multiple image upload
Drag and Drop UI
Display the image according to the markdown syntax
File type filtering

Looking for feedback.

houndci-bot · 2014-05-13T08:17:42Z

app/controllers/projects/issues_controller.rb

+    links = []
+    params['issue-imgs'].each do |img|
+      uploader.store!(img)
+      links << File.join(root_url,uploader.url)


Space missing after comma.

dzaporozhets · 2014-05-13T08:24:10Z

Can you please attach screenshot or gif animation so we can see how UI looks?

kemenaran · 2014-05-13T12:08:11Z

app/controllers/projects/issues_controller.rb

+  def upload_image
+    uploader = AttachmentUploader.new(@project.namespace.path, @project.path)
+    links = []
+    params['issue-imgs'].each do |img|


Wouldn't it be simpler to limit the upload API to a single image (rather than several images at once), and let the javascript/browser send one request per image to upload?

IMHO it would make the error handling easier on the browser side: each upload would receive its own success or error. If several files can be uploaded at once the error handling must deal with partial errors.

erbunao · 2014-05-14T02:59:23Z

@randx, here are the print screens:

Attaching image:

Result:

With invalid file type:

houndci-bot · 2014-05-14T08:12:01Z

app/controllers/projects/issues_controller.rb

+  def upload_image
+    @upload_path = File.join(project.namespace.path, @project.path)
+    @accepted_types = %w(png jpg jpeg gif)
+    uploader = FileUploader.new('uploads/'+@upload_path,@accepted_types)


Space missing after comma.
Surrounding space missing for operator '+'.

houndci-bot · 2014-05-15T00:08:38Z

config/routes.rb

@@ -300,9 +299,10 @@
        end
      end

-      resources :issues, constraints: {id: /\d+/}, except: [:destroy] do
+      resources :issues, constraints: {id: /\d+/}, except: [:destroy] do=


Space inside { missing.
Space inside } missing.

houndci-bot · 2014-05-15T00:15:56Z

app/controllers/projects/issues_controller.rb

@@ -77,7 +80,7 @@ def update
    @issue = Issues::UpdateService.new(project, current_user, params[:issue]).execute(issue)

    respond_to do |format|
-      format.js
+      format.js   


Trailing whitespace detected.

houndci-bot · 2014-05-15T05:36:39Z

app/controllers/projects/issues_controller.rb

+    links = []
+    params['issue-imgs'].each do |img|
+      alt = uploader.store!(img)
+      links << {'alt' => File.basename(alt,'.*'), 'url' => File.join(root_url, uploader.url)}


Line is too long. [93/80]
Space missing after comma.
Space inside { missing.
Space inside } missing.

erbunao · 2014-05-15T08:07:01Z

@kemenaran:
As I understand, this PR allows attachments to be uploaded for an issue - but not for Merge Requests, wiki projects, etc.

How about putting the upload API at the Project level? The project_controller would handle uploading and deleting images. And the same browser code could be used to upload an image from anywhere in a project: issues, MR, wiki, wall, all the same code.

@kemenaran, we have considered your point and started working on it. So far we are done with issue, comment, and wiki forms.

@randx, do we need to submit a separate pull request for the feature mentioned by @kemenaran or just maintain this PR.

houndci-bot · 2014-05-16T07:57:27Z

app/controllers/files_controller.rb

@@ -13,5 +13,4 @@ def download
      redirect_to uploader.url
    end
  end
-end
-
+end


Final newline missing.

mbrgm · 2014-05-16T15:55:40Z

+1!

dblessing · 2014-05-16T16:28:54Z

@erbunao I would think you want to implement within projects up front. It will be a better user experience. It seems like it would also be advantageous to have this new behavior in notes/comments as some point. Anything that can unify the experience.

Things seem to be looking good so far. From a UI standpoint I'm not sure I like the location of the invalid file type alert. It would probably be best to have it red so it matches the normal bootstrap alert/danger scheme. It may be more appropriate to have the bar appear above or below the description field or at the bottom of the screen like other error banners do. I understand it may be helpful to have the error message appear more in context so the user notices it easily.

dblessing · 2014-05-16T16:29:47Z

Also, be careful with your editor to ensure you don't have trailing whitespace and you don't remove new lines at the end of files. It'll make review and merging easier later on. :)

dblessing · 2014-05-16T16:33:10Z

app/services/issues/create_service.rb

      if issue.save
        notification_service.new_issue(issue, current_user)
        event_service.open_issue(issue, current_user)
        issue.create_cross_references!(issue.project, current_user)
        execute_hooks(issue)
      end
-


Please avoid changes like this in files where you're not adding/changing code.

dblessing · 2014-05-16T16:36:08Z

favicon.ico file seems to have been removed in this PR, also.

jvanbaarsen · 2014-05-16T19:39:21Z

app/controllers/projects/issues_controller.rb

@@ -69,7 +68,11 @@ def create
          render :new
        end
      end
-      format.js
+      format.js do |format|
+        # issue = Issues.create()


What does this comment try to explain?

erbunao · 2014-05-19T22:51:45Z

@dblessing, @jvanbaarsen, we're already refactoring the code for the feature and we're planning to submit another PR for a more generic markdown area (maybe we can incorporate this feature to the notes (comments) section and/or wiki). Thanks for the feedback!

Wachiwi · 2014-05-20T10:23:52Z

very nice feature! 👍

erbunao · 2014-05-23T09:12:49Z

@randx, @dosire, @dblessing. We submitted a new pull request which applies this feature to other markdown area. Please take a look at #7011. Thanks!

houndci-bot reviewed May 13, 2014
View reviewed changes

erbunao mentioned this pull request May 13, 2014

Add feature to be able to upload a file to an issue #6467

Closed

kemenaran reviewed May 13, 2014
View reviewed changes

houndci-bot reviewed May 14, 2014
View reviewed changes

houndci-bot reviewed May 15, 2014
View reviewed changes

Implements drag and drop upload in creating issues

e2fa8f3

houndci-bot reviewed May 15, 2014
View reviewed changes

erbunao changed the title ~~Issue file upload~~ Image upload in creating issue May 15, 2014

Reformats alternative.

f992375

houndci-bot reviewed May 15, 2014
View reviewed changes

Implements select box.

5e8ece7

houndci-bot reviewed May 16, 2014
View reviewed changes

app/controllers/files_controller.rb

@@ -13,5 +13,4 @@ def download

redirect_to uploader.url

end

end

end

end

Copy link

houndci-bot May 16, 2014

Choose a reason for hiding this comment

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

Final newline missing.

dblessing reviewed May 16, 2014
View reviewed changes

jvanbaarsen reviewed May 16, 2014
View reviewed changes

erbunao mentioned this pull request May 23, 2014

Implements drag and drop upload of image in every markdown-area #7011

Merged

erbunao closed this May 23, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image upload in creating issue #6962

Image upload in creating issue #6962

erbunao commented May 13, 2014

houndci-bot May 13, 2014

dzaporozhets commented May 13, 2014

kemenaran May 13, 2014

erbunao commented May 14, 2014

houndci-bot May 14, 2014

houndci-bot May 15, 2014

houndci-bot May 15, 2014

houndci-bot May 15, 2014

erbunao commented May 15, 2014

houndci-bot May 16, 2014

mbrgm commented May 16, 2014

dblessing commented May 16, 2014

dblessing commented May 16, 2014

dblessing May 16, 2014

dblessing commented May 16, 2014

jvanbaarsen May 16, 2014

erbunao commented May 19, 2014

Wachiwi commented May 20, 2014

erbunao commented May 23, 2014

Image upload in creating issue #6962

Image upload in creating issue #6962

Conversation

erbunao commented May 13, 2014

Choose a reason for hiding this comment

dzaporozhets commented May 13, 2014

Choose a reason for hiding this comment

erbunao commented May 14, 2014

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

erbunao commented May 15, 2014

Choose a reason for hiding this comment

mbrgm commented May 16, 2014

dblessing commented May 16, 2014

dblessing commented May 16, 2014

Choose a reason for hiding this comment

dblessing commented May 16, 2014

Choose a reason for hiding this comment

erbunao commented May 19, 2014

Wachiwi commented May 20, 2014

erbunao commented May 23, 2014