What should we do about note comments?

[gravitystorm]

This isn't the first issue on notes and note comments, and I doubt it will be the last either!

The original implementation of notes has, for unimportant reasons, left us with the situation where Notes surprisingly don't have an author or body attribute, but instead every Note has a "first comment" that we treat as special. However, this is not how anyone really thinks of a note - we all think that the note belongs to someone, and has some body text associated with the note itself. The implementation that we have though is as if a note is created out of thin air, with no text and nobody associated to it, whenever someone makes that first comment. This conceptual mismatch, or this leaky abstraction, I think has led to more bugs and errors than any other part of the codebase, and is confusing and also doesn't seem to provide any useful benefits.

There's a whole bunch of open issues and open pull requests that try to fix the various bugs that crop up due to this leaky abstraction. However, many of these are fixing the symptoms one at a time, by making the views or API code more and more complex, rather than trying to fix the underlying model. This has generally made me averse to reviewing and/or merging them, since I think they are going in the wrong direction. I'm aware however that we haven't discussed what the right destination should be!

Personally, I think the long term goal should be to remove the concept of this "special first comment" entirely, and end up with a more straightforward model where we have:

• A Note, which has a body and an author. A note can exist without any comments.
• NoteComments, none of which are "special" and each can be individually added, removed, hidden etc without affecting the validity of the Note.

This would align note comments with our other comment models, for example changeset comments and diary comments. We don't implement diary entries by having a blank DiaryEntry and a special first DiaryComment containing the text of the diary entry!

If we are all agreed on the destination, then we can start planning out the refactoring required to get there. This will also help assess the existing pull requests and open issues on this topic.

[gravitystorm]

The way that I would approach this refactoring would be roughly:

• Corral all the complexity and special casing into the models, rather than having the logic spread out over controllers and views as we currently have. So if you ask for e.g. @note.comments you would not get the special first comment in the list returned. Other things like current_user.note_comments would also behave as expected, with no special first comments that then need to be ignored.
• Rework the controllers and views to act as we would expect for the end stage, e.g. when creating a Note you provide the body and author to Note#create rather than the two-step process currently in api/notes_controller
• Create a migration to add the author and body to the Note database tables, start to use them when they are available, and start backfilling them from the special first comments. Destroy the unnecessary special first comments when they are no longer needed.
• The output of the API has to remain the same for now, of course. But the special first comments can be synthesised for API 0.6, and dropped in the next API version so that only the genuine comments are shown.

[tomhughes]

Sounds like a plan to me...

[AntonKhorev]

There's a whole bunch of open issues and open pull requests that try to fix the various bugs

There are three pull requests from me that are related to note comments:

• Don't add title to note marker if note has no visible comments #3617 fixes the javascript code that works with the API, and you're not changing the API
• Consider the first author of all note comments to be the note author #3607 fixes the model
• Show notes with no visible comments; show only first comment as note description #3608 actually does the controller-view fix. This is partially because you wouldn't accept fixes to the model, as you can see in #3607.

[danieldegroot2]

May or may not be a prerequisite for ENT8R/NotesReview#113. Feedback welcome in said issue.

[woodpeck]

Agree that @gravitystorm's plan is sensible. (I ended up here looking for a way to vent my frustration at note comments getting hidden when the author is deleted, rendering whole discussion threads utterly meaningless - unlike changeset discussions where contributions by deleted users are kept as being from "user_12345".)

One caveat though, currently note comments can be associated with an action - even closing a note without a comment will generate an empty comment as a "vehicle" for the note state change. This will have to be catered for somehow.

[tordans]

@gravitystorm how would the concept of closing/reopening notes work with this new data model?

One way would be…

• Note has a field open=true (default)
• NoteComment has a field action=null (default)|close|reopen
• Whenever a comment is saved, a hook is triggered that updates the Note.open flag.
  • Should this update be done in a Transaction to guard against edge cases where the async update fails? Eg. when two comments are submitted at the same time.
  • … or is there a Queuing system in place which could put a UpdateNoteStatus job on the queue which in turn would update the Note based on the latest NoteComment at that time (which does not have to be the NoteComment that triggered it…)

In addition, we could use the same mechanism to denormalize lastOpened_by and lastClosed_by flags on the Notes. However whoever will work on this should decide based on performance considerations if this is needed vor the UI or API output.

[gravitystorm]

• Note has a field open=true (default)

Notes already have a closed_at field.

• NoteComment has a field action=null (default)|close|reopen

NoteComments already have an event field.

• Whenever a comment is saved, a hook is triggered that updates the Note.open flag.

The API has different endpoints for opening, closing and reopening notes, and the comment needs to be sent to the right one. There is, if you look very closely, a race condition in the code (e.g. checking for whether a note has been closed already is not done with a lock on the note) but in practice that hasn't been a big issue compared to all the other issues with notes.

• or is there a Queuing system in place

Yes, we use ActiveJob both for the jobs defined in app/jobs and also for other features like ActionMailer's .deliver_later

But for something that is expected to happen instantaneously (e.g. updating a single record in the database) there's generally no need for a queue. Queuing is more useful for things that can take some time (e.g. processing 10,000 Tracepoint records) or interact with external services (e.g. delivering mail).

[kmpoppe]

• Create a migration to add the author and body to the Note database tables, start to use them when they are available, and start backfilling them from the special first comments. Destroy the unnecessary special first comments when they are no longer needed.

removed brain fog

[tomhughes]

Why would we need the notes dump? No data has been deleted, the web site is just choosing not to display it.

[kmpoppe]

Why would we need the notes dump? No data has been deleted, the web site is just choosing not to display it.

I had suspected that the data has not been deleted because it was still in the dump. I'm just gonna keep my hands off that and wait for the process to finish.

[tordans]

FYI I proposed this refactoring as the next EWG funded project following the bidding processed outlined at https://github.com/osmfoundation/ewg_bidding.

The related PR is at osmfoundation/ewg_bidding#9 with the proposed bid text in the files section which will be discussed again during the next EWG Meeting.

(Just a reminder, that this text is only the first step which outlines the work to be done (what). The next step will be a bid that outlines the proposed implementation (how) and corresponding estimate.)