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.

Sign up for GitHub

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

Jump to bottom

NT-2000: Retrying posting a failed reply #1322

Merged
merged 8 commits into from Jul 16, 2021
Merged

NT-2000: Retrying posting a failed reply #1322

merged 8 commits into from Jul 16, 2021

Conversation

leighdouglas
Copy link
Contributor

📲 What

  • Handle retrying the call to post a reply when the replying fails the first time.
  • Also handle bug that posts the reply as a root comment
  • Hide the reply button on a reply comment card
  • Linter cleanup

🛠 How

  • Update the view comment replies and reply to comment streams with the new reply comment that has a parent id - we were previously sending the old comment without the updated id.

Screen Shot 2021-07-15 at 10 16 37 AM

👀 See

retry.reply.comments.mp4

📋 QA

Reply on old root comment

  • Open a project you have backed
  • Reply to a comment on a backed project while in airplane mode
  • Turn off airplane mode and retry posting the reply
  • Reply should show success state "posted"
  • Navigate back to root comments screen and pull to refresh a few times
  • Confirm there are no duplicated comments, or that your reply was posted as a root comment
  • If no replies we posted on the root comment before yours, pull to refresh should now show "view replies"
  • Tap view replies and check that your reply is posted

Reply on new root comment

  • Open a project you have backed
  • Post a root comment in airplane mode
  • Turn off airplane mode and retry posting the root comment
  • Confirm success "posted" and tap reply
  • Turn on airplane mode again and reply to root comment
  • Turn off airplane mode and retry posting the reply
  • Reply should show success state "posted"
  • Navigate back to root comments screen and pull to refresh a few times
  • Confirm there are no duplicated comments, or that your reply was posted as a root comment
  • If no replies we posted on the root comment before yours, pull to refresh should now show "view replies"
  • Tap view replies and check that your reply is posted

Story 📖

NT-2000: Retrying posting a failed reply

Arkariang
Arkariang reviewed Jul 15, 2021
View reviewed changes
app/src/main/java/com/kickstarter/viewmodels/CommentsViewHolderViewModel.kt
@@ -339,7 +349,10 @@ interface CommentsViewHolderViewModel {
* // TODO: we will need the entire comment plus very important the [parentId]
* @return Observable<Comment>
*/
private fun executePostCommentMutation(postCommentData: PostCommentData, errorObservable: BehaviorSubject<Throwable>) =
private fun executePostCommentMutation(
Copy link
Contributor

Choose a reason for hiding this comment

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

the TODO can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍🏽

Arkariang
Arkariang reviewed Jul 15, 2021
View reviewed changes
app/src/main/java/com/kickstarter/viewmodels/CommentsViewHolderViewModel.kt Outdated
@@ -232,6 +234,7 @@ interface CommentsViewHolderViewModel {

comment
.compose(takeWhen(this.onViewCommentRepliesButtonClicked))
.map { postedSuccessfully?.value ?: it }
Copy link
Contributor

@Arkariang Arkariang Jul 15, 2021 •
edited

Choose a reason for hiding this comment

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

the value for comment and postedSuccessFully should be the same. Why do we need to map here those two values? postedSuccessfully should be use only as a way of trigering the callback to the ThreadActivity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment that was emitted from the comment thread still had the default parentId of -1, which is why the reply was being treated as a root comment. Updating to use the comment item emitted from the postedSuccessfully stream gives us the updated parentId.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest using Observable.merge on the declaration for comment observable, something like:
Screen Shot 2021-07-15 at 4 09 00 PM
in that way you don't need to update several RXChains.

I'll approve though, my comment up there is a suggested improvement 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks for catching this. Just updated 👍🏽

@codecov
Copy link

codecov bot commented Jul 15, 2021 •
edited

Codecov Report

Merging #1322 (371b4aa) into master (9a4b9d1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1322   +/-   ##
=========================================
  Coverage     74.12%   74.12%           
  Complexity      739      739           
=========================================
  Files           221      221           
  Lines          6666     6666           
  Branches        406      406           
=========================================
  Hits           4941     4941           
  Misses         1589     1589           
  Partials        136      136

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a4b9d1...371b4aa. Read the comment docs.

@leighdouglas leighdouglas force-pushed the leigh/nt-2000-reply-retry-success branch from 8b7f00e to 5c6ca10 Compare July 16, 2021 19:37
@leighdouglas leighdouglas merged commit 031e392 into master Jul 16, 2021
@leighdouglas leighdouglas deleted the leigh/nt-2000-reply-retry-success branch July 16, 2021 21:32
Arkariang added a commit that referenced this pull request Jul 19, 2021
@Arkariang
Merge branch 'imartin/NT-2124-integration-shot' of github.com:kicksta…
a8a2ed5 
…rter/android-oss into imartin/NT-2125-first-test

* 'imartin/NT-2124-integration-shot' of github.com:kickstarter/android-oss:
  NT-2000: Retrying posting a failed reply Scrolling bug (#1327)
  NT-2000: Retrying posting a failed reply (#1322)
  NT-2130 :Pull missing translations (#1326)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arkariang Arkariang Arkariang approved these changes

@hadia hadia Awaiting requested review from hadia

Assignees
No one assigned
Labels
feature ✨ UI 🎨
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@leighdouglas @Arkariang