operationsClient's removeCompletedOperations doesn't work #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tionsClient When the operations client tries to remove completed operations, it retrieves the current collection of operations, removes the completed ones, and sets the modified collection back. However, this operation doesn't actually work because the preferences services assumes that a modification is being made to the passed in collection and that the entries that have been removed from the collection should not be acted upon. To fix this, the clear flag will be defined and set to true so that the preferences service will know that the preferences should be overwritten with the passed in collection. Signed-off-by: Remy Suen <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
operationsClient'sremoveCompletedOperationsdoesn't actually remove the completed operations when the call is made to the preferences service. This is because it checks for acleardefined on theoptionsto determine whether the original preference's value should be merged with the passed in value or not.In our case, the operations list gets parsed and completed entries gets removed. Then it gets to the preferences service and the removed entries gets added back in because it believes that only the passed in values should be modified and any values that aren't passed in should just be preserved. This makes sense when thinking of preferences in the regular way but fails in the
removeCompletedOperationscase so the fix is to send in{ clear: true }as an option when trying to remove the completed operations.Note that this pull request is technically incomplete as the test doesn't seem to fail. When I comment out the fix, the test will just timeout instead of failing. The error that the assertion throws seems to get swallowed somewhere. Can someone with some knowledge about our
Deferredimplementation take a look at this?