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

Fix duplicate comments posting related to Log calls from inside processNetworkRequestQueue() #6043

Merged
merged 1 commit into from
Oct 26, 2021

Conversation

marcaaron
Copy link
Contributor

Details

There was some previous suggestion that a more long term solution to this is make report comments idempotent.
We should revisit that, but I think this bug is too obvious to not try to fix. The network code should never be calling processNetworkRequestQueue() from inside processNetworkRequestQueue().

Fixed Issues

$ #4214

Tests

  • Added an automated test that failed and showed the two report comments being created
  • Fixed the bug
  • Tests pass

QA Steps

This is difficult to test because a comment must be sent while there are 49 logs waiting to be sent.
So we can just make sure that comments can be sent normally and that no duplicates are appearing.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

@marcaaron marcaaron self-assigned this Oct 25, 2021
@marcaaron marcaaron requested a review from a team as a code owner October 25, 2021 18:21
@MelvinBot MelvinBot requested review from sketchydroide and removed request for a team October 25, 2021 18:22
Copy link
Contributor

@sketchydroide sketchydroide left a comment

Choose a reason for hiding this comment

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

LGTM

@sketchydroide sketchydroide merged commit 10529c9 into main Oct 26, 2021
@sketchydroide sketchydroide deleted the marcaaron-shouldProcessImmediately branch October 26, 2021 11:21
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @sketchydroide in version: 1.1.9-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@marcaaron
Copy link
Contributor Author

This is a pass for me on staging.

  • Left multiple comments on a report
  • Saw that the Log command executed.
  • No duplicate comments

2021-10-26_07-32-33

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.10-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants