Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Raise error when note has no comments #2176

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jguthrie100
Copy link
Contributor

Temporary fix for #2146

  • Raises RecordNotFound exception when note has no comments (avoiding 500 Error)

@tomhughes
Copy link
Member

This definitely needs a test but more significantly I'm not sure this is really the right fix. I have a feeling that the visible scope on notes should exclude these but that turns out to be non-trivial to do...

Still thinking about the best way to deal with it really.

@jguthrie100
Copy link
Contributor Author

The problem is that @note_comments is nil, but in the view, a request is made for @note_comments.first, which then causes a crash.

So either the view needs to be rewritten to handle a case where there are no comments, or the view needs to not be called in the first place..

@jguthrie100
Copy link
Contributor Author

The line that crashes is:

<div class="note-description">
<%= h(@note_comments.first.body.to_html) %>
</div>

@jguthrie100
Copy link
Contributor Author

jguthrie100 commented Mar 16, 2019

Unless a new error is raised: ActiveRecord::RecordDeleted, which opens the comments page, but notes the comments that have been deleted?

@jguthrie100
Copy link
Contributor Author

This definitely needs a test but more significantly I'm not sure this is really the right fix. I have a feeling that the visible scope on notes should exclude these but that turns out to be non-trivial to do...

What do you mean by "exclude these"? What should be excluded?

@mmd-osm
Copy link
Contributor

mmd-osm commented Dec 4, 2022

#3608 supersedes this pull request.

@gravitystorm gravitystorm added the notes Related to the notes feature label Dec 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes Related to the notes feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants