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

[MBL-820] Update Privacy viewmodel to RX2 #2119

Merged
merged 4 commits into from
Sep 3, 2024
Merged

[MBL-820] Update Privacy viewmodel to RX2 #2119

merged 4 commits into from
Sep 3, 2024

Conversation

mtgriego
Copy link
Contributor

@mtgriego mtgriego commented Sep 3, 2024

📲 What

Updating Privacy Viewmodel to RX2

🤔 Why

Need to get rid of RX1

📋 QA

Go to the privacy activity and confirm everything works as expected

Story 📖

MBL-820

@mtgriego mtgriego self-assigned this Sep 3, 2024
@mtgriego mtgriego marked this pull request as ready for review September 3, 2024 17:50
@codecov-commenter
Copy link

codecov-commenter commented Sep 3, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 73.33333% with 16 lines in your changes missing coverage. Please review.

Project coverage is 67.61%. Comparing base (36b994c) to head (cec08f8).

Files with missing lines Patch % Lines
...ava/com/kickstarter/viewmodels/PrivacyViewModel.kt 73.33% 5 Missing and 11 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2119      +/-   ##
============================================
- Coverage     67.64%   67.61%   -0.03%     
- Complexity     2193     2194       +1     
============================================
  Files           367      367              
  Lines         22579    22597      +18     
  Branches       3240     3248       +8     
============================================
+ Hits          15273    15279       +6     
- Misses         5569     5574       +5     
- Partials       1737     1744       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

.filter { it.isNotNull() }
.map { user -> user.createdProjectsCount().isNonZero() }
.subscribe(this.hidePrivateProfileRow)
.filter { it.isNotNull() && it.getValue().isNotNull() }
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldnt this cause a crash if it / currentUser is null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, since the operation happens in order it.isNotNull checks it first, then if it is null the filter fails since its an 'and' check, if it is not null, the second check happens to cover the getValue value

fun T?.isNotNull(): Boolean {
return this != null
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, what i mean is that im concerned with the first null check. to me, this is implying that the item could be null before hitting that first check, and my understanding is that any null value that's passed down the pipeline is an issue with rxjava 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the concept is called short circuiting. Basically since its an && it will evaluate the operations in order and if any fail along the way it doesnt continue and just marks the result as false

Copy link
Contributor

Choose a reason for hiding this comment

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

got it! thank you for explaining 👍🏽

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, what i mean is that im concerned with the first null check. to me, this is implying that the item could be null before hitting that first check, and my understanding is that any null value that's passed down the pipeline is an issue with rxjava 2

I believe RX2 doesnt like nulls, but as long as you dont emit them you are okay. I could be wrong about that, but since in this case we are just grabbing an observable im just covering the very odd case its null so that the subsequent it.getValue does not crash

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can remove them if you like

Copy link
Contributor

@leighdouglas leighdouglas left a comment

Choose a reason for hiding this comment

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

🎉

@mtgriego mtgriego merged commit 794c7f9 into master Sep 3, 2024
3 checks passed
@mtgriego mtgriego deleted the mtg/MBL-820 branch September 3, 2024 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants