Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
Closed in favor of #5568, #5579 and #5609. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#issuecomment-2640909060 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
Closed #5511. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#event-16215594698 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 6 commits. 690d392bd866a411bed93aded9b18f884bcfcc21 Adds optional using of notes records 9e70276e1997d22e7e4cf496b87157749bb9e468 Adds helper routine note_description 38fde849ca894f1d8f21118bf2c7730c76c178a2 Replaces using description with note_description ce61e316e3678709922c3feaf4ec461aab235409 Removes note's author from RSS 0e60e2e71579dff7b7b163f08bafd278deb9caae Removes dropping note's first comment 3d2870c146803e52269b6a9cb6ebede89fecb8c4 Don't add title to note marker if note has no visible comments -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/283125648b4ae86d4918add763fc515461691206..3d2870c146803e52269b6a9cb6ebede89fecb8c4 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 6 commits. bc47525980dcbeb7604a76a141e9c59e65a67f8b Adds optional using of notes records 762a6dd2562cca5fc2e3f6ac81fb3db747c0fa30 Adds helper routine note_description bd1af6bfbd70026bae3e798da811eecc5b68258a Replaces using description with note_description 3a09e3eb07f1c2387980ce9a4c21609d471cfaae Removes note's author from RSS 2a0ad8ba64f54e96748f10383f5217bfe1cd2ac0 Removes dropping note's first comment 283125648b4ae86d4918add763fc515461691206 Don't add title to note marker if note has no visible comments -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/d791420fa4113f9b77a7749a7c3ef19cc1b168e6..283125648b4ae86d4918add763fc515461691206 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request. > @@ -7,6 +7,7 @@ end json.properties do json.id note.id + json.description note_description(note) Thanks, postponed to some future PR. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1932637965 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request. > - def test_description -comment = create(:note_comment) -assert_equal comment.body, comment.note.description - -user = create(:user) -comment = create(:note_comment, :author => user) -assert_equal comment.body, comment.note.description - end Thanks, fixed! -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1932637321 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request. > - # Return the note's description, derived from the first comment - def description -comments.first.body - end Thanks, fixed. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1932636732 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request. >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 + association(:author).reader Thanks, fixed! -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1932635828 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
> The commits here are doing things in a strange order. One way to untangle > them is to make another pull request that drops author_id/_ip without doing > other things. Added #5568 The problem with the current solution is JS is breaking (displaying only a subset of notes from bounding box) when trying to display note without visible comments (e.g. we create new user, login as that user, create new note without further commenting / making other actions, delete user, navigate to area where previously created note is located). This bug is solved by #3617 but it's still not merged. Shall we wait #3617 to be merged or add its solution to this PR or deal with these problematic notes in some other way? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#issuecomment-2619703679 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 5 commits. 9c250186827ebd03019136b7e03b89e02cfe291c Adds optional using of notes records b8be2499b03fcfbe488a6644a3bc6d2839984a4f Adds helper routine note_description af5ed1047a15d262cb06f96d9f813ee97b97000f Replaces using description with note_description b59e20494e557af4412ee457ad38beeccb4f4fab Removes note's author from RSS d791420fa4113f9b77a7749a7c3ef19cc1b168e6 Removes dropping note's first comment -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/4fa79a72eae42a5c82e8073cbfd64d777f853964..d791420fa4113f9b77a7749a7c3ef19cc1b168e6 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. > @@ -7,6 +7,7 @@ end json.properties do json.id note.id + json.description note_description(note) Let's suppose you have a normal note. No deleted users touched it. From the API point of view its description is in the first comment. If you add a description like here, you'll have it twice in the API output. You can't remove it from the first comment because it's not backwards compatible. Does it make sense to have it twice? And that's not the only way to fix the tooltip problem. You could instead add some attribute to an API comment that indicates if it's a description or not. In other words, you shouldn't do this here because it's a completely different problem. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1931817739 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
The commits here are doing things in a strange order. One way to untangle them is to make another pull request that drops author_id/_ip without doing other things. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#issuecomment-2618412474 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. > @@ -7,6 +7,7 @@ end json.properties do json.id note.id + json.description note_description(note) Why are we changing the API? Did anyone suggested changing the API? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2577547564 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. > - def test_description -comment = create(:note_comment) -assert_equal comment.body, comment.note.description - -user = create(:user) -comment = create(:note_comment, :author => user) -assert_equal comment.body, comment.note.description - end You dropped `note.description` a few commits before. Why did you keep the tests until this commit? + same for `note.author_id` and `author_ip` -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2577511169 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. > - # Return the note's description, derived from the first comment - def description -comments.first.body - end Don't you use this method in the helper before this commit and the helper assumes it returns the migrated description? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2577444695 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. >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 + association(:author).reader Is this going to work? ```suggestion super ``` `association` is [not documented](https://github.com/rails/rails/blob/cf6ff17e9a3c6c1139040b519a341f55f0be16cf/activerecord/lib/active_record/associations.rb#L51). -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2577387803 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 9 commits. 26b3a557d0e1ebe4e58db6613a29e4607953eaad Adds optional using of notes records c0371ab00eba60ccac109a65145695cd259db1ca Added helper routine note_description a1747d6d306c8a28151c24654349b3140101232c Replaced using description with note_description 2e5288961234ce34acf16c2e33b17ac80869e62c Removed unnecessary methods from Note e963ad6244d11370257d77b4a4e6abcb75b6040d Removed note's author from RSS 6defaa9ca7dc93cd3777079922de649b244ec469 Adds displaying first comment as a comment 0456efa0a1c99977115eae7766b56505a638e2dc Updated note's unit tests d603cca45b8f48ac4cfc8d111eeeb5520bface8c Adds note's description to note's XML / JSON 4fa79a72eae42a5c82e8073cbfd64d777f853964 Adds using description for note tooltip -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/14a78ae879966ece2917dc34d858ddbf05c87d8e..4fa79a72eae42a5c82e8073cbfd64d777f853964 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev @tomhughes do you have any further comment / request on this PR? Thanks! -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#issuecomment-2616343008 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 9 commits. 354ff7345d672b582ad4ab4a500cb6c5ec99d9b3 Adds optional using of notes records 0fd95b34d2e62ce5cc5cef7c375408561a9138eb Added helper routine note_description 5989ea88418f6e087fcf3591e48470cca45a4295 Replaced using description with note_description 230998c0575174adef38efde63815d64bedf10c2 Removed unnecessary methods from Note b3f6f19b71ad12cc1abcebbbe6bf72cceec60eed Removed note's author from RSS 4c1774786b8297ab6dba2d6fc54c4fec4251eb5f Adds displaying first comment as a comment b213f6f28cc6bbd03bfdeab7599f88fe630942ab Updated note's unit tests 564420e390b9776d40eea678816c40ab64081894 Adds note's description to note's XML / JSON 14a78ae879966ece2917dc34d858ddbf05c87d8e Adds using description for note tooltip -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/4fc089e8bc80de44aad2c8c7c29cb19959d66503..14a78ae879966ece2917dc34d858ddbf05c87d8e You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request.
>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
Added 2 commits solving [above two
problems](https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1927277396).
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1928669893
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 8 commits. dd7351f38fe3011592da001e5d21bce6f7f79ed9 Added helper routine note_description f654d29d8d73591cbd4255cd12145f0e9019a4d9 Replaced using description with note_description 5ccab1b63361a68025c953f9a3e2f3acff5477ea Removed unnecessary methods from Note 742597d3b990038018102112cbef337f13828ac4 Removed note's author from RSS dcde901f76972b6c63ac8e2626b8e54630b69a1d Adds displaying first comment as a comment 256c77338dd3d7bb618639dcf0584c4bc764884f Updated note's unit tests 691e914ead4eb8e5eab435459e8860bb8984091b Adds note's description to note's XML / JSON 4fc089e8bc80de44aad2c8c7c29cb19959d66503 Adds using description for note tooltip -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/e3f82ea6eecb34e4a8631358a8b03c0691ed4dcc..4fc089e8bc80de44aad2c8c7c29cb19959d66503 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request.
>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
I've just pushed the latest changes without solving
[above](https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1927277396).
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1927783796
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 5 commits. 855ad4a0b09ebfb89f200dcc2064de71497460f7 Added helper routine note_description f4b173f4f15340e45bfbf787828d6df0415881a3 Replaced using description with note_description 9ed20bea95eb3f7969cfe4cad76510da4cbdc7f9 Removed unnecessary methods from Note 41e41fd9b26208fa0b3af1624284f939ab6cab0f Removed note's author from RSS e3f82ea6eecb34e4a8631358a8b03c0691ed4dcc Updated note's unit tests -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/180ffe452e9642c8f4935da43ddb1b7d27f47209..e3f82ea6eecb34e4a8631358a8b03c0691ed4dcc You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 6 commits. b3f9efc95650bec172400793bb94dcb31d77de69 Adds optional using of notes records fc83eec9d7b27de7be23784f475d6deda06a309c Added helper routine note_description 8e64da58a34cd78a2e214edbd5ca0a5da5434456 Replaced using description with note_description 8d6d58148981df20a90dc1aac4706089d66b83d5 Removed unnecessary methods from Note 2a3508a9f53ad4575d0459e5a09be4cca2e55db3 Removed note's author from RSS 180ffe452e9642c8f4935da43ddb1b7d27f47209 Updated note's unit tests -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/aee9a1043232a30488e944e5647b70004329994b..180ffe452e9642c8f4935da43ddb1b7d27f47209 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request.
>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
This opens 2 problems with notes with deleted users:
- 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?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1927277396
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request.
>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:
https://github.com/openstreetmap/openstreetmap-website/pull/5494/commits/2c19c2143ec51e507d19b41fb6c89454b3d83dfc.
In #3607 I also [removed the deleted author](
https://github.com/openstreetmap/openstreetmap-website/pull/3607/files#diff-9b8b3e6d46a2a808a27e4c2f8d88c5470d39ee77146f8bb530124c0cacc35dbeR16)
from rss.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2568828414
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request.
>def description
-comments.first.body
+if user_ip.nil? && user_id.nil?
+ comments.first.body
+else
+ RichText.new("text", self[:description])
+end
end
Thanks! Started displaying "deleted" if note's author is deleted.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1925993418
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request. >def author_id -comments.first.author_id +if user_ip.nil? && user_id.nil? + comments.first.author_id +else + user_id +end end Thanks! Started using `all_comments` instead of `comments`. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1925986399 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request. >end - # Return the note's author IP address, derived from the first comment + # Return the note's author IP address, unless record is unavailable and + # it will be derived from the first comment def author_ip Thanks! Removed both `author_ip` and `author_id` methods since they are not used anywhere. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1925985816 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 2 commits. 58971ce24ab40e29f82b38ed62ad0ce66fb42a53 Adds optional use of notes records aee9a1043232a30488e944e5647b70004329994b Updated Note model unit tests -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/04bc2d4b540d6438d506afd351a0fc3c1f063aa9..aee9a1043232a30488e944e5647b70004329994b You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. >end - # Return the note's author IP address, derived from the first comment + # Return the note's author IP address, unless record is unavailable and + # it will be derived from the first comment def author_ip Do we need this method at all? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2565725347 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. >def author_id -comments.first.author_id +if user_ip.nil? && user_id.nil? + comments.first.author_id +else + user_id +end end Now you are revealing the true author as soon as migration happens, but not before. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1923977398 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request.
>def description
-comments.first.body
+if user_ip.nil? && user_id.nil?
+ comments.first.body
+else
+ RichText.new("text", self[:description])
+end
end
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)
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1923944473
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request.
>def description
-comments.first.body
+if user_ip.nil? && user_id.nil?
+ comments.first.body
+else
+ RichText.new("text", self[:description])
+end
end
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](https://github.com/nenad-vujicic/openstreetmap-website/commit/0cc2d59311479c56e009e0e63b4bdb54113aeb61)),
but can easily switch to another logic?
Btw, before data-migration PR we planned 2 more steps ([updating
creating](https://github.com/nenad-vujicic/openstreetmap-website/commit/8df2f38e21c0703dfb49267d2b6a3f9bc808a63b)
and [updating
searching](https://github.com/nenad-vujicic/openstreetmap-website/commit/75212422a99134aa0b67fac4f7c1533f9ffba15d)).
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1923645103
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic commented on this pull request. >def author_id -comments.first.author_id +if user_ip.nil? && user_id.nil? + comments.first.author_id +else + user_id +end end The latest push should fix this. Thanks! -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1922162897 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 1 commit. 04bc2d4b540d6438d506afd351a0fc3c1f063aa9 Adds optional use of notes records -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/a6d27220424cb6293d4b572477c53ef7313cc3c1..04bc2d4b540d6438d506afd351a0fc3c1f063aa9 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@tomhughes commented on this pull request.
>def description
-comments.first.body
+if user_ip.nil? && user_id.nil?
+ comments.first.body
+else
+ RichText.new("text", self[:description])
+end
end
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1921582791
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request.
>def description
-comments.first.body
+if user_ip.nil? && user_id.nil?
+ comments.first.body
+else
+ RichText.new("text", self[:description])
+end
end
Weren't you insisting on not showing the true description until LWG agrees?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1921578658
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@tomhughes commented on this pull request.
>def description
-comments.first.body
+if user_ip.nil? && user_id.nil?
+ comments.first.body
+else
+ RichText.new("text", self[:description])
+end
end
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?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#discussion_r1921574801
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
> 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.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#issuecomment-2600887365
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request. >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. -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2561026988 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@AntonKhorev commented on this pull request.
>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.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2561025712
You are receiving this because you are subscribed to this thread.
Message ID:
___
rails-dev mailing list
rails-dev@openstreetmap.org
https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@nenad-vujicic pushed 2 commits. bb9d30fef5a295b42bffaf0cfe49329e9fb9989b Adds optional use of notes records 57b066df587a29ffe6f79db8da9061be839dc7f6 small fix -- View it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511/files/31dbd76c9cf4ec2e7d11673f0a272bab8d615807..57b066df587a29ffe6f79db8da9061be839dc7f6 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
Re: [openstreetmap/openstreetmap-website] Adds optional use of notes records (PR #5511)
@tomhughes commented on this pull request. Why does this need to use `[:user_id]` etc rather than the more typical `.user_id` form? -- Reply to this email directly or view it on GitHub: https://github.com/openstreetmap/openstreetmap-website/pull/5511#pullrequestreview-2560409294 You are receiving this because you are subscribed to this thread. Message ID: ___ rails-dev mailing list rails-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/rails-dev
