Adds optional use of notes records #5511

nenad-vujicic · 2025-01-17T12:09:27Z

Description

PR adds use of description, user_id, user_ip from notes table if data-migration is done, otherwise use body, author_id, author_ip from note's first comment. Decision procedure "data-migration is done" is defined with "is user_ip or user_id not nil?".

This PR is 4th from set of PRs described here.

How has this been tested?

Automated unit tests and manual testing.

tomhughes

Why does this need to use [:user_id] etc rather than the more typical .user_id form?

nenad-vujicic · 2025-01-19T14:35:20Z

Why does this need to use [:user_id] etc rather than the more typical .user_id form?

I replaced using [:user_id] with user_id (same with user_ip), but had to leave self[:description] because of possible recursion. I couldn't use self.{user_id, user_ip, description} because of Rubocop warnings.

AntonKhorev · 2025-01-19T14:45:57Z

app/models/note.rb

  def description
-    comments.first.body
+    if user_ip.nil? && user_id.nil?
+      comments.first.body
+    else
+      RichText.new("text", self[:description])
+    end
  end


I assume that self[:description] is going to be the true note description, the text note was created with. comments.first.body is not necessarily that text because comments.first is not necessarily the first api 0.6 comment. comments only contain api 0.6 comments by active users and doesn't contain api 0.6 comments of deleted users.

We could make it consistent by using all_comments instead but that obviously changes the behaviour from what we have now, but then that's going to be the case anyway when this transition is completed?

Weren't you insisting on not showing the true description until LWG agrees?

I don't think I've said that as such.

What I have said is that we need a proper documented policy about how to handle deleted users and what we should and shouldn't show. I don't think I specifically asked for that to come from LWG though obviously that may be needed to answer questions about what we can and can't do.

Have we reached a decision on this? We planned to copy the body of the first note's comment (i.e. one with event set to "opened") to note's description (data migration-script), but can easily switch to another logic?

Btw, before data-migration PR we planned 2 more steps (updating creating and updating searching).

Copying is not the problem, the problem is you won't be able to the description if the user who created it is deleted. You'll have to remember that whenever you access note.description.

There was a draft policy by DWG about deleted users that was supposed to be sent to LWG but wasn't. It says that DWG recommends note comments of deleted users to be visible. If that's impossible, "a comment is not shown because ..." is to be displayed instead. Descriptions are not mentioned but the same should apply to them.

The default solution if no decision was made was to replace description with "deleted" when the note is displayed:

https://github.com/openstreetmap/openstreetmap-website/pull/4481/files#diff-74d80704129ce910bd38a56b250122e716028ac0085b3627ea8875b9216b65ceR25-R29

Since you already started using note.description, one of the following things should happen:

you remove deleted descriptions here in the model

you remove them on use

we actually do a proper documented policy (in other words it's easier to remove deleted descriptions for now)

Thanks! Started displaying "deleted" if note's author is deleted.

AntonKhorev · 2025-01-19T14:51:21Z

app/models/note.rb

  def author_id
-    comments.first.author_id
+    if user_ip.nil? && user_id.nil?
+      comments.first.author_id
+    else
+      user_id
+    end
  end


If user_id is a true id of the user who created the note, it won't necessarily match author.id, see my comment for description. Attempts to fix that for author were made in the past but were rejected by the maintainers, see for example #3607.

The latest push should fix this. Thanks!

Now you are revealing the true author as soon as migration happens, but not before.

Thanks! Started using all_comments instead of comments.

app/models/note.rb

AntonKhorev · 2025-01-23T04:21:26Z

app/models/note.rb

  def description
-    comments.first.body
+    if !author.nil? && author.status == "deleted"
+      RichText.new("text", I18n.t("notes.show.description_when_author_is_deleted"))
+    elsif user_ip.nil? && user_id.nil?
+      comments.first.body
+    else
+      RichText.new("text", self[:description])
+    end
  end

-  # Return the note's author object, derived from the first comment
+  # Return the note's author object, unless record is unavailable and
+  # it will be derived from the first comment
  def author
-    comments.first.author
-  end
-
-  # Return the note's author ID, derived from the first comment
-  def author_id
-    comments.first.author_id
-  end
-
-  # Return the note's author IP address, derived from the first comment
-  def author_ip
-    comments.first.author_ip
+    if user_ip.nil? && user_id.nil?
+      all_comments.first.author
+    else
+      self[:author]
+    end
  end


If author returns the true author, description should also return the true description. There's note_author helper which hides the author when necessary. Similarly you can make a helper that hides the description, and you won't need I18n.t("notes.show.description_when_author_is_deleted") inside the model. And you'll use the helper in these places: 2c19c21.

In #3607 I also removed the deleted author from rss.

This opens 2 problems with notes with deleted author:

When we click on such note, it will show "deleted" as a description, but first next comment will be lost (i.e. we need to update app/views/notes/show.html.erb not to drop first comment if author is deleted) and

When we hover over such note, it will show first "note.comments" comment instead of "deleted".

Should we fix these in this PR or in next?

I've just pushed the latest changes without solving above.

Added 2 commits solving above two problems.

Adds author association and optional use of note's records. If data-migration is done, real author is returned, otherwise if data-migration is not done, first note's comment author is returned. Added method author_deleted for encapsulating checking if note's author is deleted.

Added new helper routine note_description for retrieving note's description. Helper routine returns "deleted" if author is deleted, first comment's body if data-migration is not done or note's description record if data-migration is done.

Replaced using note's description method with note_description helper routine.

Removed author_id, author_ip and description methods from Note model because they are not used anymore.

Removed writing note's author to note's RSS if note's author is deleted.

Adds displaying first (visible) comment as a comment in case of deleted note's author. After adding displaying "deleted" as note description, first comment is not anymore dropped when displaying note's comments.

Adds description field to generated note's XML and JSON files with note's description.

Adds initializing note's tooltips from description retrieved from JSON file.

tomhughes reviewed Jan 18, 2025

View reviewed changes

nenad-vujicic force-pushed the issue_replace_opening_comment_task_4 branch 2 times, most recently from 57b066d to a6d2722 Compare January 19, 2025 14:23

AntonKhorev reviewed Jan 19, 2025

View reviewed changes

nenad-vujicic force-pushed the issue_replace_opening_comment_task_4 branch from a6d2722 to 04bc2d4 Compare January 20, 2025 08:50

AntonKhorev reviewed Jan 21, 2025

View reviewed changes

app/models/note.rb Outdated Show resolved Hide resolved

nenad-vujicic force-pushed the issue_replace_opening_comment_task_4 branch from 04bc2d4 to aee9a10 Compare January 22, 2025 21:12

AntonKhorev reviewed Jan 23, 2025

View reviewed changes

nenad-vujicic force-pushed the issue_replace_opening_comment_task_4 branch 2 times, most recently from 180ffe4 to e3f82ea Compare January 23, 2025 22:21

nenad-vujicic mentioned this pull request Jan 24, 2025

Moves the note's description, author ID, and author IP from the first comment to the note itself #5485

Draft

nenad-vujicic force-pushed the issue_replace_opening_comment_task_4 branch from e3f82ea to 4fc089e Compare January 24, 2025 13:03

nenad-vujicic added 9 commits January 24, 2025 16:25

Added helper routine note_description

0fd95b3

Added new helper routine note_description for retrieving note's description. Helper routine returns "deleted" if author is deleted, first comment's body if data-migration is not done or note's description record if data-migration is done.

Replaced using description with note_description

5989ea8

Replaced using note's description method with note_description helper routine.

Removed unnecessary methods from Note

230998c

Removed author_id, author_ip and description methods from Note model because they are not used anymore.

Removed note's author from RSS

b3f6f19

Removed writing note's author to note's RSS if note's author is deleted.

Adds displaying first comment as a comment

4c17747

Adds displaying first (visible) comment as a comment in case of deleted note's author. After adding displaying "deleted" as note description, first comment is not anymore dropped when displaying note's comments.

Updated note's unit tests

b213f6f

Adds note's description to note's XML / JSON

564420e

Adds description field to generated note's XML and JSON files with note's description.

Adds using description for note tooltip

14a78ae

Adds initializing note's tooltips from description retrieved from JSON file.

nenad-vujicic force-pushed the issue_replace_opening_comment_task_4 branch from 4fc089e to 14a78ae Compare January 24, 2025 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds optional use of notes records #5511

Adds optional use of notes records #5511

nenad-vujicic commented Jan 17, 2025

tomhughes left a comment

nenad-vujicic commented Jan 19, 2025

AntonKhorev Jan 19, 2025

tomhughes Jan 19, 2025

AntonKhorev Jan 19, 2025

tomhughes Jan 19, 2025

nenad-vujicic Jan 21, 2025

AntonKhorev Jan 21, 2025

nenad-vujicic Jan 22, 2025

AntonKhorev Jan 19, 2025

nenad-vujicic Jan 20, 2025

AntonKhorev Jan 21, 2025

nenad-vujicic Jan 22, 2025

AntonKhorev Jan 23, 2025

nenad-vujicic Jan 23, 2025 •

edited

Loading

nenad-vujicic Jan 23, 2025

nenad-vujicic Jan 24, 2025

Adds optional use of notes records #5511

Are you sure you want to change the base?

Adds optional use of notes records #5511

Conversation

nenad-vujicic commented Jan 17, 2025

Description

How has this been tested?

tomhughes left a comment

Choose a reason for hiding this comment

nenad-vujicic commented Jan 19, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nenad-vujicic Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nenad-vujicic Jan 23, 2025 •

edited

Loading