Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add comment local ids to OnCommentChanged events #250

Merged
merged 3 commits into from
Dec 20, 2016

Conversation

maxme
Copy link
Contributor

@maxme maxme commented Dec 8, 2016

No description provided.

@@ -399,6 +407,7 @@ private void pushComment(RemoteCommentPayload payload) {
if (payload.comment == null) {
OnCommentChanged event = new OnCommentChanged(0);
event.causeOfChange = CommentAction.PUSH_COMMENT;
event.changedCommentsLocalIds.add(payload.comment.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will throw an NPE as payload.comment is null here.

@aforcier
Copy link
Contributor

It's possible for the CommentModel to be null in most if not all of these cases - we should be null-checking it, or using @NonNull where possible.

@maxme
Copy link
Contributor Author

maxme commented Dec 19, 2016

It's possible for the CommentModel to be null in most if not all of these cases - we should be null-checking it, or using @nonnull where possible.

Fixed in 5ceb77d - I wasn't able to make the CommentModel @NonNull in RemoteCommentPayload because it can be used to pull a comment by its site+remoteId only.

@@ -309,6 +307,7 @@ private void updateComment(CommentModel payload) {
rowsAffected = CommentSqlUtils.insertOrUpdateComment(payload);
}
OnCommentChanged event = new OnCommentChanged(rowsAffected);
event.changedCommentsLocalIds.add(payload.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be null checked as well, since an UPDATE_COMMENT action can legitimately be called from a FluxC client (and be null). This applies to the payload.isError() check in this same method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I'm not sure about null checking all payloads in all stores. I don't know if we should add @NonNull to our actions.

@@ -317,12 +316,14 @@ private void removeComment(CommentModel payload) {
int rowsAffected = CommentSqlUtils.removeComment(payload);
OnCommentChanged event = new OnCommentChanged(rowsAffected);
event.causeOfChange = CommentAction.REMOVE_COMMENT;
event.changedCommentsLocalIds.add(payload.getId());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above comment, payload could be null.

@aforcier
Copy link
Contributor

:shipit:

@aforcier aforcier merged commit fe0ac3e into develop Dec 20, 2016
@aforcier aforcier deleted the issue/add-local-ids-to-oncommentchanged-events branch December 20, 2016 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants