• Marin Jankovski's avatar
    Merge branch 'fix-edit-note-with-votes' into 'master' · 66fa4b09
    Marin Jankovski authored
    Fix server error when editing a note to "+1" or "-1"
    
    ### Summary
    
    If a user edits a comment with "+1" or "-1" in the beginning, the POST returns an Internal Server error. (issue #1151). This merge request resolves that error.
    
    ### Steps to reproduce
    
    1. Comment on an issue with "Test comment".
    2. Edit the issue.
    3. Write "+1" and click "Save Comment".
    
    ### Expected behavior
    
    The edited note should be saved and refreshed. Any previous upvotes/downvotes from the user should contain a strikethrough.
    
    ### Observed behavior
    
    Internal Error
    
    ### Relevant logs
    
    ```
    Started PUT "/avocode/avocode-manager/notes/4996" for 185.33.136.107 at 2015-02-28 17:11:53 +0100
    Processing by Projects::NotesController#update as JS
    Parameters: {"utf8"=>"✓", "authenticity_token"=>"*removed*", "note"=>{"note"=>"+1\r\n\r\nYes"}, "commit"=>"Save Comment", "project_id"=>"avocode/avocode-manager", "id"=>"4996"}
    Completed 500 Internal Server Error in 86ms
    ActionView::Template::Error (undefined method `each' for nil:NilClass):
    28: %span.note-last-update
    29: = note_timestamp(note)
    30:
    31: - if note.superceded?(@notes)
    32: - if note.upvote?
    33: %span.vote.upvote.label.label-gray.strikethrough
    34: %i.fa.fa-thumbs-up
    app/models/note.rb:495:in `superceded?'
    app/views/projects/notes/_note.html.haml:31:in `_app_views_projects_notes__note_html_haml___812277000516355462_69988235638820'
    app/controllers/projects/notes_controller.rb:71:in `note_to_html'
    app/controllers/projects/notes_controller.rb:103:in `render_note_json'
    app/controllers/projects/notes_controller.rb:39:in `block (2 levels) in update'
    app/controllers/projects/notes_controller.rb:38:in `update'
    ```
    
    ### Fix
    
    It turns out no tests were present for the "Edit Issue" functionality. I added spinach tests to exercise this and reproduced the error.
    
    Most of the routes in `notes_controller.rb` appear to render all notes for the given discussion. `_form.html.haml` needs the full list of notes commented by the user to add strikethroughs for older upvotes/downvotes. However, only the `index` route appeared to obtain this information. The fix is to add a `before_filter` to obtain all the user's notes beforehand, except in the delete case where this information is not needed.
    
    Things to watch: `NotesFinder` needs `target_type` and `target_id` to determine what to do. I'm not sure if there is a conscious effort to phase these keywords out in favor of `noteable_type` and `noteable_id`.
    
    See merge request !360
    66fa4b09
Name
Last commit
Last update
app Loading commit data...
bin Loading commit data...
config Loading commit data...
db Loading commit data...
doc Loading commit data...
docker Loading commit data...
features Loading commit data...
lib Loading commit data...
log Loading commit data...
public Loading commit data...
safe Loading commit data...
spec Loading commit data...
tmp Loading commit data...
vendor/assets Loading commit data...
.foreman Loading commit data...
.gitattributes Loading commit data...
.gitignore Loading commit data...
.hound.yml Loading commit data...
.pkgr.yml Loading commit data...
.rspec Loading commit data...
.rubocop.yml Loading commit data...
.ruby-version Loading commit data...
.simplecov Loading commit data...
.teatro.yml Loading commit data...
CHANGELOG Loading commit data...
CONTRIBUTING.md Loading commit data...
GITLAB_SHELL_VERSION Loading commit data...
Gemfile Loading commit data...
Gemfile.lock Loading commit data...
Guardfile Loading commit data...
LICENSE Loading commit data...
MAINTENANCE.md Loading commit data...
PROCESS.md Loading commit data...
Procfile Loading commit data...
README.md Loading commit data...
Rakefile Loading commit data...
VERSION Loading commit data...
config.ru Loading commit data...