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

Return early if no valid sequenceNumber can be found #2777

Merged
merged 1 commit into from
May 11, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented May 11, 2021

cc @roryabraham

Details

Updates a check that only checks for an "undefined" sequenceNumber when it should be ignoring any invalid values for a sequenceNumber. Seems like this PR actually did not make it to production. But I also happened to spot a null value in reportMaxSequenceNumbers on dev so I think this is a good change regardless.

Fixed Issues

No issue, but related to #2704

Tests

QA Steps

Same as #2704

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@marcaaron marcaaron requested a review from a team as a code owner May 11, 2021 00:30
@marcaaron marcaaron self-assigned this May 11, 2021
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team May 11, 2021 00:30
Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

Looks good!

// update the last read. Most likely, we have just created the report and it has no comments. But we should err on
// the side of caution and do nothing in this case.
if (_.isUndefined(sequenceNumber)
&& (!reportMaxSequenceNumbers[reportID] && reportMaxSequenceNumbers[reportID] !== 0)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, but why not use ??

(reportMaxSequenceNumbers[reportID] ?? 0) !== 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why should we use it? 😄

Copy link
Contributor Author

@marcaaron marcaaron May 11, 2021

Choose a reason for hiding this comment

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

Here's the result I want...

Only reject the value if undefined or null but not 0

and figure that out by looking for something falsy

!reportMaxSequenceNumbers[reportID]

and (&&)

that is not 0

reportMaxSequenceNumbers[reportID] !== 0

since if reportMaxSequenceNumbers[reportID] was 0 it would be falsy.

@jasperhuangg jasperhuangg merged commit 158ad1e into main May 11, 2021
@jasperhuangg jasperhuangg deleted the marcaaron-checkForNullReport branch May 11, 2021 01:24
@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.41-8🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.44-0🚀

platform result
🤖 android 🤖 failure ❌
🖥 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