Fix CLICK-to-Upload Image Cross-Wiring Issues #8987

noi5e · 2021-01-11T03:54:23Z

I think the plan I described in the issue above is actually working! e.target is now accurate in the start fileupload functions. which means we can use it to assign $D.selected. That means that every time a user successfully uploads a file by clicking on the upload button, there's a much higher likelihood (if not 100%) that it's going to go into the appropriate box.

Summary:

slight refactoring of dragdrop.js's drop eventListener
assign $D.selected to e.target in dragdrop.js's start fileupload function
some refactoring in editor.js's wrap and refresh methods
new system tests:
- IMMEDIATE image CLICK to upload in REPLY comment form works
- IMMEDIATE image CLICK to upload in EDIT comment form works
- image CLICK to upload into EDIT form isn't CROSS-WIRED with MAIN form
- image CLICK to upload into EDIT form isn't CROSS-WIRED with REPLY form

(This issue is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)

gitpod-io · 2021-01-11T03:54:27Z

codeclimate · 2021-01-11T03:55:20Z

Code Climate has analyzed commit ca17ce3 and detected 0 issues on this pull request.

View more on Code Climate.

noi5e · 2021-01-11T04:00:21Z

app/assets/javascripts/dragdrop.js

@@ -33,25 +33,27 @@ jQuery(function() {
  });
  $('.dropzone').on('drop',function(e) {
    // this 'drop' listener is also reused for pages with just one form, ie. /wiki/new
-    $D.selected = $(e.target).closest('div.comment-form-wrapper').eq(0);
+    const closestCommentFormWrapper = e.target.closest('div.comment-form-wrapper'); // this returns null if there is no match
    e.preventDefault();
    let params = {};


I rewrote this part for greater readability, using JavaScript's closest (as opposed to jQuery's method with the same name).

JavaScript's closest returns null, whereas jQuery's returns an object. I thought this would be more intuitive.

codecov · 2021-01-11T04:00:48Z

Codecov Report

❗ No coverage uploaded for pull request base (main@626c26c). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #8987   +/-   ##
=======================================
  Coverage        ?   82.03%           
=======================================
  Files           ?      100           
  Lines           ?     5956           
  Branches        ?        0           
=======================================
  Hits            ?     4886           
  Misses          ?     1070           
  Partials        ?        0

noi5e · 2021-01-11T04:03:32Z

app/assets/javascripts/dragdrop.js

+          // for click-upload-button scenarios, it's important to set $D.selected here, because the 'drop' listener above doesn't run in those:
+          $D.selected = $(e.target).closest('div.comment-form-wrapper');
+          // the above line is redundant in drag & drop, because it's assigned in 'drop' listener too.
+          // on /wiki/new & /wiki/edit, $D.selected will = undefined from this assignment


This is a major change: assign $D.selected in the start upload function.

As a reminder, this function runs on both click and drop image upload events. It's important to assign $D.selected here for click events, because the drop listener doesn't run for those.

I left a comment in the code about this, but I'm thinking it would be good down the road to not have to assign $D.selected twice in a row for drop events. Not sure how to do that yet.

noi5e · 2021-01-11T04:05:08Z

app/assets/javascripts/editor.js

    args['textarea'] = args['textarea'] || 'text-input'
    $E.textarea = $('#'+args['textarea'])
-    $E.title = $('#title')
+    $E.textarea.bind('input propertychange',$E.save)
+    // preview
    args['preview'] = args['preview'] || 'preview'


No additions here, just rearranged stuff for greater readability. Sometime soon, I'd like to rewrite this part to use JavaScript ES6's default parameters: initialize(textarea = 'text-input')

noi5e · 2021-01-11T04:07:12Z

app/assets/javascripts/editor.js

+    $E.textarea = ($D.selected).find('textarea').eq(0);
+    $E.textarea.bind('input propertychange',$E.save);
+    // preview
+    $E.preview = ($D.selected).find('#preview').eq(0);
  },


I didn't think these lines needed to be inside of a conditional checking for $D.selected (it made it little less readable). I moved the conditional into $E.wrap

noi5e · 2021-01-11T04:13:49Z

app/assets/javascripts/editor.js

+    // we don't need to refresh $E's values if we're on a page with a single comment form, ie. /wiki/new or /wiki/edit
+    if (window.location.pathname !== "/wiki/new" && !isWikiCommentPage && $D.selected) {
+      this.refresh();
+    }
    var len = $E.textarea.val().length;


This is a pretty big change too. The (window.location + '').includes('wiki') part was originally done in #5650 to fix image upload in /wiki/new and /wiki/{wiki name}/edit...

Unfortunately this had the side effect of running refresh on wiki comments pages (which include('wiki') in the window.location). I finally figured out this doesn't need to happen, because the wiki#show view just loads comments through note#comments. This used to make writing system tests more difficult because things ran differently for wiki comments.

The last note is that I'm thinking that this.refresh(); may not need to be called at all. We could remove it entirely if we assign $D.selected in all the right places, and if $E.textarea remains constant on /wiki/new and /wiki/edit. But that's for another PR.

noi5e · 2021-01-11T04:16:14Z

test/system/comment_test.rb

      visit get_path(page_type, nodes(node_name).path)
-      # Create a comment
-      main_comment_form =  page.find('h4', text: /Post comment|Post Comment/).find(:xpath, '..') # title text on wikis is 'Post comment'
-      # fill out the comment form


I'm trying to prune the system tests bit-by-bit. Every time we post a comment manually where we don't need to do so is extra runtime. Here node.add_comment can save some time.

jywarren

Looks great! Ready to merge? Thanks!!!

noi5e · 2021-01-11T17:20:18Z

@jywarren Yep, ready to merge!

sagarpreet-chadha

Fantastic !!

jywarren · 2021-01-11T20:17:43Z

🎉🎉🎉

* add new image upload system tests * rename comment fixtures * assign D.selected in start; rewrite drop assignment publiclab#8926 * specify E.refresh conditional call in E.wrap publiclab#8926

noi5e added 4 commits January 10, 2021 19:38

add new image upload system tests

f0caafc

rename comment fixtures

34340bd

assign D.selected in start; rewrite drop assignment publiclab#8926

a83d08f

specify E.refresh conditional call in E.wrap publiclab#8926

ca17ce3

noi5e changed the title ~~Bug: Fix CLICK-to-Upload Image Cross-Wiring Issues (Fixes #8926)~~ Bug: Fix CLICK-to-Upload Image Cross-Wiring Issues Jan 11, 2021

noi5e commented Jan 11, 2021

View reviewed changes

This was referenced Jan 11, 2021

[Discussion] Add Unique HTML IDs to Comment Forms to Fix Image Upload Cross-Wiring Issues #8926

Closed

OLD CLOSED Comment Editor Overhaul Issue #8775

Closed

noi5e requested review from jywarren, cesswairimu and sagarpreet-chadha January 11, 2021 09:26

noi5e added bug the issue is regarding one of our programs which faces problems when a certain task is executed outreachy testing issues are usually for adding unit tests, integration tests or any other tests for a feature labels Jan 11, 2021

noi5e added this to the [Outreachy] Comment Editor Winter 2020 milestone Jan 11, 2021

jywarren approved these changes Jan 11, 2021

View reviewed changes

sagarpreet-chadha approved these changes Jan 11, 2021

View reviewed changes

jywarren merged commit cdbdd7d into publiclab:main Jan 11, 2021

This was referenced Jan 20, 2021

[System Tests] Increase System Tests Speed by Decreasing Coverage #9045

Merged

Comment Editor Overhaul Project [Outreachy] #9069

Open

noi5e changed the title ~~Bug: Fix CLICK-to-Upload Image Cross-Wiring Issues~~ Fix CLICK-to-Upload Image Cross-Wiring Issues Jan 23, 2021

noi5e deleted the bug/image-click-upload-crosswiring branch January 27, 2021 02:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CLICK-to-Upload Image Cross-Wiring Issues #8987

Fix CLICK-to-Upload Image Cross-Wiring Issues #8987

noi5e commented Jan 11, 2021 •

edited

gitpod-io bot commented Jan 11, 2021

codeclimate bot commented Jan 11, 2021

noi5e Jan 11, 2021

codecov bot commented Jan 11, 2021 •

edited

noi5e Jan 11, 2021

noi5e Jan 11, 2021

noi5e Jan 11, 2021

noi5e Jan 11, 2021 •

edited

noi5e Jan 11, 2021

jywarren left a comment

noi5e commented Jan 11, 2021

sagarpreet-chadha left a comment

jywarren commented Jan 11, 2021

Fix CLICK-to-Upload Image Cross-Wiring Issues #8987

Fix CLICK-to-Upload Image Cross-Wiring Issues #8987

Conversation

noi5e commented Jan 11, 2021 • edited

gitpod-io bot commented Jan 11, 2021

codeclimate bot commented Jan 11, 2021

noi5e Jan 11, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 11, 2021 • edited

Codecov Report

noi5e Jan 11, 2021

Choose a reason for hiding this comment

noi5e Jan 11, 2021

Choose a reason for hiding this comment

noi5e Jan 11, 2021

Choose a reason for hiding this comment

noi5e Jan 11, 2021 • edited

Choose a reason for hiding this comment

noi5e Jan 11, 2021

Choose a reason for hiding this comment

jywarren left a comment

Choose a reason for hiding this comment

noi5e commented Jan 11, 2021

sagarpreet-chadha left a comment

Choose a reason for hiding this comment

jywarren commented Jan 11, 2021

noi5e commented Jan 11, 2021 •

edited

codecov bot commented Jan 11, 2021 •

edited

noi5e Jan 11, 2021 •

edited