Fix Image Drag & Drop Cross-Wiring in Edit Comment Form #8897
Conversation
noi5e
added
testing
|
|
Codecov Report
@@ Coverage Diff @@
## main #8897 +/- ##
=======================================
Coverage ? 81.94%
=======================================
Files ? 100
Lines ? 5955
Branches ? 0
=======================================
Hits ? 4880
Misses ? 1075
Partials ? 0 |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
noi5e
changed the title
|
Did some more research, I've refined my understanding of what happens here. The questions above still apply so please check them out! What Scripts Run WhereI was mistaken before about what runs where. Looking at plots2/app/views/comments/_edit.html.erb Line 72 in cd59bfc So I think my assumption before that Scenario: BugIf I sit down and break down the case where the bug occurs, I think it goes like this:
Those are my working assumptions so far. I think it explains why the error doesn't happen on wikis. Going to map out the success case (where the URL goes into the edit comment form like expected), and then from there think about how to fix this one. |
|
Yeah, I think my scenario for the bug is accurate. Here's the scenario for the success case: Scenario: Success
|
This comment has been minimized.
This comment has been minimized.
|
|
||
| $('#c<%= comment.id%>div').bind('drop',function(e) { | ||
| e.preventDefault(); | ||
| $D.selected = $(e.target).closest('div.comment-form-wrapper').eq(0); |
There was a problem hiding this comment.
Notice this additional line!
There was a problem hiding this comment.
I wrapped everything in _edit.html.erb in a div.comment-form-wrapper.
The other major change is that now $D.selected is assigned in this eventListener bound to #c1div.
A major part of the drag-and-drop crosswiring is fixed. My tests (including the new one I wrote for this issue) are passing. I had to rewrite some of the system tests in this most recent push: 'edit comment' and 'delete comment' rely on .comment-form-wrapper for some reason.
There was a problem hiding this comment.
Oops, the tests aren't passing. I'll take a closer look tomorrow.
This comment has been minimized.
This comment has been minimized.
|
This is looking awesome, i am thinking that some of the keys to bear in mind may be:
I hope some of this helps, i know you are very likely already thinking about a lot of this. You're doing an excellent job and unraveling some really subtle and messy threads... and reducing technical debt! No need to tackle every optimization or ideal code structure at once, but i feel that making smaller pragmatic changes, adding tests to document our steps, and then slowly and iteratively working towards a better and more legible structure is probably the best way to go. Good work unraveling this without getting overwhelmed by the scope!!! |
OK, you found the duplicated code already but I think it could be good to look at the blame view of plots2/app/assets/javascripts/dragdrop.js Line 44 in cd59bfc plots2/app/views/comments/_edit.html.erb Line 72 in cd59bfc dragdrop.js -- but not in this PR, let's think about that as a separate task!
I guess just that given that we use JS to build forms once new comments are added via AJAX, we might start to consider if we want that to be the only way we create comment forms -- that is, consolidate with a
Definitely do things one at a time in separate PRs. And, once you've really grasped what is /supposed/ to happen, write a descriptively-named test for it -- even if it's a pretty small thing! At least add a comment to the test to explain the step -- a good example might be I always feel that it's hard to keep a complex system in my head, so i try not to. If it takes a lot of time to recover a mental state -- get oriented in the code -- that's a sign of code which should be broken down into simpler more descriptive parts, using function names and tests to map things out and become more legible.
Here, I'd look at the blame view for that wiki switching code and try to trace back who added it and why... i know wiki comments were added much later than others, but i don't totally understand why they'd behave differently in this scenario! OK, hope this helps!!!! 👍 again, great work!!! |
noi5e
changed the title
…osswiring' into bug/edit-comment-select-image-crosswiring
![codeclimate[bot]](https://avatars.githubusercontent.com/in/9403?s=60&v=4){kind=small}
|
Code Climate has analyzed commit 42f2e05 and detected 0 issues on this pull request. View more on Code Climate. |
|
😌 Phew. I think this is it. Leaving my last few notes.
|
|
@jywarren This one's ready to merge! It passes all the tests, and I've checked it out locally as well by playing with image upload on comments and /wiki/new. I feel very confident it's not creating any new bugs. Sorry this is such a big PR with so many long notes. I probably could've broken out the new Just to summarize, here is what this PR is doing:
|
| $('.dropzone').on('drop',function(e) { | ||
| $D.selected = $(e.target).closest('div.comment-form-wrapper').eq(0); | ||
| e.preventDefault(); | ||
| let params = {}; |
There was a problem hiding this comment.
This part is new, and significant. Rather than just comment out $E, I rewrote this initialization call to account for two different scenarios:
- pages where there's only one comment form (ie. /wiki/new). we want this to be initialized to #text-input.
- pages with multiple comments-- here we want $E.textarea to be the closest textarea to the drop event.
Like I've said, I do want to refactor this method and others so that we don't have to $E.initialize, but I feel like this is a very good compromise for now.
|
OK, i'm going through this comment by comment, so taking notes in my comment as i go... reading through #8897 (comment), I wanted to note that some of the confusion from
This is definitely a sign that we should try to compartmentalize the logic more. Nobody should be juggling all these things in their head at once -- it's a sign of bad design! 😄
Did you find this via a test failure? If not, perhaps we should ensure it's covered, because our longer-term desire to redesign the initialization system may mean we're gonna start tearing things out to rebuild... and we'll want good test coverage to do that with confidence. But no worries, we won't do this in this PR most likely! Basically after this point, you did great... your guess at breaking a test, your docs re: 3 different bugs... all good.
great catch! This can be saved for later, let's just be organized about remembering things to circle back to. Re: hacky fix, that's 👍 totally fine as you point out we'll come back here to fix more soon... just leave a note to self! In general, to avoid such large PRs, what if we have the principle that we split out any bugs we find that aren't related to the core function of this PR, into their own issues (or even just to checkboxes on the planning issue for now).
exactly! OK, and finished reading your comments, and all the rest sound super. I really appreciate your including your reasonings here, as I think it'll provide helpful context if we need to circle back at some point. And in general, +1000000 rewriting Merging this now, MANY thanks for such careful work, and congrats! This is also bringing into much sharper focus the architectural refinements we hope for down the road, their relationship to existing code, tests, and legibility! 🎉 🎉 🎉 🎉 🎉 |
| $D.selected = $(e.target).closest('div.comment-form-wrapper').eq(0); | ||
| e.preventDefault(); | ||
| let params = {}; | ||
| if ($D.selected.hasOwnProperty(0)) { |
There was a problem hiding this comment.
This logic is a bit obscure, hasOwnProperty(0) but might be improved with an explanatory comment. I think it also shows us that $D may also need refactoring along similar lines to $E.
And, apologies as I believe I was originally responsible for both $D and $E - and wow, i've grown so much as a coder in the ~10 years or so since they were written!!! And, to be frank, so has JavaScript!
There was a problem hiding this comment.
Noted, and agreed, will write some comments here to explain!
I did notice you wrote a lot of $D and $E, no worries! 😄 I'm seeing what goes into growing and maintaining a 10+ year codebase, which is such a valuable learning experience. It's a real achievement that this repo has been growing for so long!
It's kind of neat how much of this work is historical. Like it was interesting to realize that the editor code was written for creating new posts first, then repurposed for comments. I understood the code a lot more then.
| @@ -186,15 +186,70 @@ def get_path(page_type, path) | |||
| page.assert_selector('#preview img', count: 1) | |||
| end | |||
|
|
|||
| # sometimes if edit and reply/main comment forms are open, | |||
|
Thanks @jywarren again for sifting through this very large PR! It was very satisfying to
Definitely, yeah, will do this in the future! I'm starting to understand how to break up really complex issues into smaller more manageable PRs. I think before I was a little intimidated at the thought of managing merge conflicts, but now I see that's way more desirable than the alternative.
Hmm, I don't think so. All the system tests were passing at that point, because we don't have that kind of test coverage. Definitely noted! This will be covered in the next few PRs, which are focused on click-to-upload issues.
Noted! Will make a small follow-up PR to leave comments, and change the |
noi5e
changed the title
…ab#8897) * add test for comment image crosswiring * wrap everything in comment-form-wrapper, assign .selected in drop listener (publiclab#8670) * delete defunct function, fix indentation & spacing publiclab#8670 * rewrite tests that rely on comment-form-wrapper * remove deprecated code, comments; fix indent * initialize on pageload (publiclab#8670) * rewrite for multiple file inputs * change test to focus on image drag & drop publiclab#8670 * restore E.initialize call in drop publiclab#8670 * undo E.initialize call publiclab#8670 * assign unique IDs to comment form textareas publiclab#8670 * change ID selectors to class ones to match new IDs publiclab#8670 * fix indentation publiclab#8670 * change ID reference to class reference publiclab#8670 * initialize E with parameters in drop listener publiclab#8670 * update tests for new text-input selectors publiclab#8670 * change ID text-input reference to class ref publiclab#8670 * change text-input ID to class publiclab#8670 * drop: initialize E with params publiclab#8670 * remove semicolon
…dlers to .on (publiclab#8982) * change .bind eventHandlers to on publiclab#8897 * write explanatory comments publiclab#8897
…ab#8897) * add test for comment image crosswiring * wrap everything in comment-form-wrapper, assign .selected in drop listener (publiclab#8670) * delete defunct function, fix indentation & spacing publiclab#8670 * rewrite tests that rely on comment-form-wrapper * remove deprecated code, comments; fix indent * initialize on pageload (publiclab#8670) * rewrite for multiple file inputs * change test to focus on image drag & drop publiclab#8670 * restore E.initialize call in drop publiclab#8670 * undo E.initialize call publiclab#8670 * assign unique IDs to comment form textareas publiclab#8670 * change ID selectors to class ones to match new IDs publiclab#8670 * fix indentation publiclab#8670 * change ID reference to class reference publiclab#8670 * initialize E with parameters in drop listener publiclab#8670 * update tests for new text-input selectors publiclab#8670 * change ID text-input reference to class ref publiclab#8670 * change text-input ID to class publiclab#8670 * drop: initialize E with params publiclab#8670 * remove semicolon
…dlers to .on (publiclab#8982) * change .bind eventHandlers to on publiclab#8897 * write explanatory comments publiclab#8897
…ab#8897) * add test for comment image crosswiring * wrap everything in comment-form-wrapper, assign .selected in drop listener (publiclab#8670) * delete defunct function, fix indentation & spacing publiclab#8670 * rewrite tests that rely on comment-form-wrapper * remove deprecated code, comments; fix indent * initialize on pageload (publiclab#8670) * rewrite for multiple file inputs * change test to focus on image drag & drop publiclab#8670 * restore E.initialize call in drop publiclab#8670 * undo E.initialize call publiclab#8670 * assign unique IDs to comment form textareas publiclab#8670 * change ID selectors to class ones to match new IDs publiclab#8670 * fix indentation publiclab#8670 * change ID reference to class reference publiclab#8670 * initialize E with parameters in drop listener publiclab#8670 * update tests for new text-input selectors publiclab#8670 * change ID text-input reference to class ref publiclab#8670 * change text-input ID to class publiclab#8670 * drop: initialize E with params publiclab#8670 * remove semicolon
…dlers to .on (publiclab#8982) * change .bind eventHandlers to on publiclab#8897 * write explanatory comments publiclab#8897
…ab#8897) * add test for comment image crosswiring * wrap everything in comment-form-wrapper, assign .selected in drop listener (publiclab#8670) * delete defunct function, fix indentation & spacing publiclab#8670 * rewrite tests that rely on comment-form-wrapper * remove deprecated code, comments; fix indent * initialize on pageload (publiclab#8670) * rewrite for multiple file inputs * change test to focus on image drag & drop publiclab#8670 * restore E.initialize call in drop publiclab#8670 * undo E.initialize call publiclab#8670 * assign unique IDs to comment form textareas publiclab#8670 * change ID selectors to class ones to match new IDs publiclab#8670 * fix indentation publiclab#8670 * change ID reference to class reference publiclab#8670 * initialize E with parameters in drop listener publiclab#8670 * update tests for new text-input selectors publiclab#8670 * change ID text-input reference to class ref publiclab#8670 * change text-input ID to class publiclab#8670 * drop: initialize E with params publiclab#8670 * remove semicolon
…dlers to .on (publiclab#8982) * change .bind eventHandlers to on publiclab#8897 * write explanatory comments publiclab#8897
This is a PR in progress! Don't merge yet. Eventually it will fix #8670. Right now, just looking for feedback from @jywarren.
Essentially this will fix a bug that occurs when uploading images to the 'Edit Comment' form. Sometimes the image will end up in the main comment form, not the edit comment form. (Read the issue comments for more details)
So far, I've written a system test that consistently reproduces this behavior (see Files Changed).
I'm writing up a comment right now summarizing my research, why I think this bug happens, and questions about where to go from here.
(This PR is part of the larger Comment Editor Overhaul Project with Outreachy. Refer to Planning Issue #9069 for more context)