Cleaned up reply functionality #2
Conversation
There was a problem hiding this comment.
Looks much better overall. I think some of the widget code could use some refactoring. Let's discuss in a video call.
Can you also run dart format on all of this code? There are a bunch of changes that are just indentation, and if we can keep all of the code properly formatted, it'll simplify the PR to just actual changes.
| final commentsResponse = | ||
| (await _connection.select('comment', where: {'poll_id': pollId})) | ||
| (await _connection.select('comment', where: {'poll_id': pollId, 'parent_id' : parentId})) |
There was a problem hiding this comment.
For this endpoint, I believe we want to fetch all of the top-level comments for the given poll. In that case, the value for parent_id should be null. The utility function (select) doesn't support null values, so I'd recommend writing out the query like on line 29.
There was a problem hiding this comment.
Yes, I did realise that select doesn't support null values, so I instead made the parent_id 0 for top-level comments. I realise this is kind of hack-ish, but it was simpler at the time. Is this bad practice - do you suggest I change it to something more like line 29?
| @@ -13,6 +13,7 @@ class FeedService { | |||
|
|
|||
| const FeedService(this._connection); | |||
|
|
|||
| //if don't want replies in feed, then need to get rows from comment where parent_id is null | |||
There was a problem hiding this comment.
Add a condition
parent_id is null
There was a problem hiding this comment.
static const _randomFeedQuery = 'select c.id, p.id as poll_id, c.comment, p.topic as poll_topic, ' 'p.emoji as poll_emoji ' 'from comment c tablesample SYSTEM_ROWS(10) where parent_id = 0' 'join poll p on c.poll_id = p.id ' 'where p.end > @now and (select count(1) from vote v ' 'where v.user_id = @user_id and v.comment_id = c.id) = 0';
I added the condition like this - not sure if this is where it's supposed to go. I'm not too familiar w psql, but this is where I think it'd probably go?
|
|
||
| Map<String,dynamic> get details => |
There was a problem hiding this comment.
Probably the best way to do this would be to make a parent class for both poll and comment to have it contain common functions between them.
Currently, details is a function which treats both comments and polls in the same way so that the app can display text on either the poll or comment page without needing to write a separate dart file.
| @@ -48,9 +52,18 @@ class Comment extends CommentBase { | |||
|
|
|||
| factory Comment.fromJson(Json json) => _$CommentFromJson(json); | |||
|
|
|||
| Map<String,dynamic> get details => | |||
There was a problem hiding this comment.
Same as for common/lib/models/poll.dart Map<String,dynamic> get details
| @@ -39,6 +42,7 @@ class Comment extends CommentBase { | |||
| const Comment({ | |||
| required this.id, | |||
| required this.pollId, | |||
| required this.parentId, | |||
There was a problem hiding this comment.
Since this field is nullable, don't mark it with required.
There was a problem hiding this comment.
Done! I did this in multiple places, so will go back and try to catch 'em all.
| @@ -7,10 +7,10 @@ part 'poll_details.g.dart'; | |||
|
|
|||
| @JsonSerializable() | |||
| class PollDetailsResponse { | |||
| final Poll poll; | |||
| final List<Comment> comments; | |||
| final Poll parent; | |||
There was a problem hiding this comment.
Do these fields need to be renamed? PollDetailsResponse should just contain the top-level comments and there should be another request/response to fetch replies.
There was a problem hiding this comment.
In poll_details.dart in app/lib/widgets/poll/details, when we fetch comments in _fetchComments(), the server request fetches the parents (either a poll or comment) and its associated messages (either top-level comments or replies). I didn't want to have to refer to them with different names (since poll/top-level comment and top-level comment/replies has a similar relationship) so I renamed the fields as parent (poll/top-level comment) and messages (top-level comment/replies). This could also be fixed by having a parent class from which both poll and comment inherit from.
| @@ -7,8 +7,9 @@ part 'add_comment.g.dart'; | |||
| class AddCommentRequest { | |||
| final int pollId; | |||
| final String comment; | |||
|
|
|||
| const AddCommentRequest({required this.pollId, required this.comment}); | |||
| final int? parentId; //null if top-level comment | |||
There was a problem hiding this comment.
Can you rewrite this comment as a Dartdoc comment?
See here for an example.
There was a problem hiding this comment.
Oops, that's a mistake. I later changed parentId to be zero for top-level comments. if this is bad practice we can change to null, so I'll hold off on the Dartdoc comment for now.
| Iterable<Comment>? _moderationQueue; | ||
| Iterable<Comment>? _approvedComments; | ||
|
|
||
| Map<String, dynamic>? _parentText; |
There was a problem hiding this comment.
These changes are a bit confusing to me. Let's discuss this.
There was a problem hiding this comment.
This linked to your previous question about Map<String,dynamic> get details =>. As mentioned in my response to that question, I want a uniform way to display the poll and comment details (for example, at the top of the poll/details page, we want to display the poll title/description. Similarly, at the top of the comment/replies page, we want to display the top-level comment.) Map<String,dynamic> get details => does this, and I store it in _parentText.
Eliminated duplication in reply functionality by reusing widgets for polls and comments, and adding a "parent_id" field to the comment model. Could maybe be cleaner by having separate files, but it was relatively straightforward to implement replies by having certain widgets keep track of whether they are displaying a comment or a reply. If it would be a lot cleaner to separate out the widgets, that shouldn't be too difficult.