Skip to content

Follow Up to PR #2068: ensure internalEnqueue can't add the same operation#2076

Merged
jkasten2 merged 1 commit intomainfrom
fix/op-repo-loadSavedOperations-dup-bug
May 7, 2024
Merged

Follow Up to PR #2068: ensure internalEnqueue can't add the same operation#2076
jkasten2 merged 1 commit intomainfrom
fix/op-repo-loadSavedOperations-dup-bug

Conversation

@jkasten2
Copy link
Copy Markdown
Member

@jkasten2 jkasten2 commented May 7, 2024

Description

One Line Summary

Fixes a bug with the unreleased PR #2068, where OperationRepo.queue would have duplicated operations on the first clean start.

Details

This fixes a bug where loadSavedOperations would duplicate operations in OperationRepo.queue. This is because it is possible for operations to be added before it is started and these are also persisted to disk.

Motivation

The duplicate operations would cause the OperationRepo to stop processing requests, which causes all User and Subscription creates and updates to stop working.

Scope

Only affects the operation repo enqueue logic.

Testing

Unit testing

Added a new Unit test for loadSavedOperations, ensuring it doesn't duplicate requests. Also OperationRepoTest's mockOperationModelStore now simulates a store, with an in memory implementation.

Manual testing

Test on Android 14 with a clean install, ensure the logcat error is no long there. Also that we are now able to get a OneSignal id in orignal scenario.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
    • If it is hard to explain how any codes changes are related to each other then it most likely needs to be more than one PR
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
    • Simplify with less code, followed by splitting up code into well named functions and variables, followed by adding comments to the code.
  • I have reviewed this PR myself, ensuring it meets each checklist item
    • WIP (Work In Progress) is ok, but explain what is still in progress and what you would like feedback on. Start the PR title with "WIP" to indicate this.

This change is Reviewable

This fixes a bug where loadSavedOperations would duplicate operations
in OperationRepo.queue. This is because it is possible for operations
to be added before it is started and these are also persisted to disk.

Improved OperationRepoTests by making mockOperationModelStore modify
a in memory list. This was required to write a test to ensure
loadSavedOperations wasn't duplicating operations.
@jkasten2 jkasten2 force-pushed the fix/op-repo-loadSavedOperations-dup-bug branch from 86e1cf6 to 6e9b690 Compare May 7, 2024 00:51
Copy link
Copy Markdown
Contributor

@jinliu9508 jinliu9508 left a comment

Choose a reason for hiding this comment

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

Good solution!

I have tested that the operation queue no longer contains duplicate operation and no more error has occurred.
image

@jkasten2 jkasten2 merged commit eb284ef into main May 7, 2024
@jkasten2 jkasten2 deleted the fix/op-repo-loadSavedOperations-dup-bug branch May 7, 2024 16:29
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.

2 participants