Fix/gene review false #534
Conversation
| ...colorOptions, | ||
| hideLeftBorder: colorOptions.hideLeftBorder ?? false, |
There was a problem hiding this comment.
John suggested the change because:
- We already check if
colorOptionsisundefined - moving
hideLeftBorderlogic under...colorOptionsensures its overridden with the correct value
| const newKeys = currentKeys.filter(x => !listItemKeys.includes(x)); | ||
|
|
||
| // Only update if newKeys differ from addedListItemKeys | ||
| const sameLength = newKeys.length === addedListItemKeys.length; |
There was a problem hiding this comment.
Yes! I created this PR before it was approved/merged. Hope it doesn't create any issues
| @@ -98,7 +98,9 @@ export class FirebaseGeneReviewService { | |||
| const { hugoSymbol } = parseFirebaseGenePath(firebasePath) ?? {}; | |||
|
|
|||
| let updateObject = this.getGeneUpdateObject(updateValue, updatedReview!, firebasePath, uuid!); | |||
| const metaUpdateObject = this.firebaseMetaService.getUpdateObject(hugoSymbol!, isGermline, { [uuid!]: !isChangeReverted }); | |||
| const metaUpdateObject = this.firebaseMetaService.getUpdateObject(hugoSymbol!, isGermline, { | |||
There was a problem hiding this comment.
Makes sense to me, but would you mind just checking with Calvin? Feels like setting !isChangeReverted and never expecting the value to be false is a weird oversight. Figuring Calvin may have had a use case in mind
There was a problem hiding this comment.
We can go ahead with this change. If a change was reverted, we do want to remove the uuid from the meta collection because it no longer needs review. Fine with this change
ReshmaRamaiah10
force-pushed
the
fix/gene-review-false
branch
from
83b006d to
1208005
Compare
| @@ -98,7 +98,9 @@ export class FirebaseGeneReviewService { | |||
| const { hugoSymbol } = parseFirebaseGenePath(firebasePath) ?? {}; | |||
|
|
|||
| let updateObject = this.getGeneUpdateObject(updateValue, updatedReview!, firebasePath, uuid!); | |||
| const metaUpdateObject = this.firebaseMetaService.getUpdateObject(hugoSymbol!, isGermline, { [uuid!]: !isChangeReverted }); | |||
| const metaUpdateObject = this.firebaseMetaService.getUpdateObject(hugoSymbol!, isGermline, { | |||
There was a problem hiding this comment.
We can go ahead with this change. If a change was reverted, we do want to remove the uuid from the meta collection because it no longer needs review. Fine with this change
| @@ -98,7 +98,9 @@ export class FirebaseGeneReviewService { | |||
| const { hugoSymbol } = parseFirebaseGenePath(firebasePath) ?? {}; | |||
|
|
|||
| let updateObject = this.getGeneUpdateObject(updateValue, updatedReview!, firebasePath, uuid!); | |||
| const metaUpdateObject = this.firebaseMetaService.getUpdateObject(hugoSymbol!, isGermline, { [uuid!]: !isChangeReverted }); | |||
| const metaUpdateObject = this.firebaseMetaService.getUpdateObject(hugoSymbol!, isGermline, { | |||
| [uuid!]: !isChangeReverted ? true : null, | |||
There was a problem hiding this comment.
We should really check if uuid is undefined/null and then generate one if it does not exist instead of using [uuid!], this will cause null: true to be inserted.
There was a problem hiding this comment.
Is if (uuid != null) enough? I think I’ve seen null: true used somewhere before, not sure if it was from legacy platform.
There was a problem hiding this comment.
I made an issue for this. I think we can merge this if we're unsure how the UUID is becoming null.
I want to see if we can make an alert in datadog.
https://github.com/oncokb/oncokb-pipeline/issues/860
https://github.com/oncokb/oncokb-pipeline/issues/829
When a change is made, a
uuidis added to the review object with a value oftrue. This indicates that a review is required. Once the change is reviewed, the uuid’s value is set tonull, which triggers the removal of the UUID from the review object.However, if the change is reverted before being reviewed,
isChangeRevertedis set totrue.[uuid]: !isChangeRevertedresults inuuid: false, which prevents the UUID from being removed, causing the system to still show that a review is required.Fix: If
isChangeRevertedistrue, theuuidis set to null[uuid]: !isChangeReverted ? true : null