Adds optional use of notes records #5511
Conversation
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
2 times, most recently
from
57b066d to
a6d2722
Compare
I replaced using |
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
from
a6d2722 to
04bc2d4
Compare
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
from
04bc2d4 to
aee9a10
Compare
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
2 times, most recently
from
180ffe4 to
e3f82ea
Compare
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
2 times, most recently
from
4fc089e to
14a78ae
Compare
|
@AntonKhorev @tomhughes do you have any further comment / request on this PR? Thanks! |
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
from
14a78ae to
4fa79a7
Compare
app/models/note.rb
Outdated
| if user_ip.nil? && user_id.nil? | ||
| all_comments.first.author | ||
| else | ||
| association(:author).reader |
There was a problem hiding this comment.
Is this going to work?
| association(:author).reader | |
| super |
association is not documented.
There was a problem hiding this comment.
Thanks, fixed!
app/models/note.rb
Outdated
| # Return the note's description, derived from the first comment | ||
| def description | ||
| comments.first.body | ||
| end |
There was a problem hiding this comment.
Don't you use this method in the helper before this commit and the helper assumes it returns the migrated description?
There was a problem hiding this comment.
Thanks, fixed.
test/models/note_test.rb
Outdated
| 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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Thanks, fixed!
| @@ -7,6 +7,7 @@ end | |||
|
|
|||
| json.properties do | |||
| json.id note.id | |||
| json.description note_description(note) | |||
There was a problem hiding this comment.
Why are we changing the API? Did anyone suggested changing the API?
I guess you want to fix tooltips in the note layer, but you don't have to do it here.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks, postponed to some future PR.
|
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. |
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
from
4fa79a7 to
d791420
Compare
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? |
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
from
d791420 to
2831256
Compare
Adds author association and optional use of note's records. If data-migration is done, real note's author (description) is returned, otherwise if data-migration is not done, first note's comment author (body) is returned. Adds method author_deleted? for encapsulating checking if note's author is deleted.
Adds 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.
Replaces using note's description method with note_description helper routine.
Removes writing note's author to note's RSS if note's author is deleted.
Removes dropping note's first visible comment in case of deleted note's author. After adding displaying "deleted" as note's description, first visible comment is now displayed as note's comments.
nenad-vujicic
force-pushed
the
issue_replace_opening_comment_task_4
branch
from
2831256 to
3d2870c
Compare
Description
PR adds use of
description,user_id,user_ipfromnotestable if data-migration is done, otherwise usebody,author_id,author_ipfrom 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.