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-1029: Refresh Discover page after a user is blocked #1885

Merged
merged 4 commits into from
Nov 16, 2023

Conversation

amy-at-kickstarter
Copy link
Contributor

📲 What

Once you block a user, anywhere in the app, the Discover page will be refreshed.

🤔 Why

Once you've blocked someone, we want to stop showing that person's content. Refreshing the Discover page means that we'll re-fetch the list of projects, filtering out any projects by blocked users.

⏰ TODO

If I throw in alogEvent(), it indicates that my signal is hooked up correctly, but I don't think it's actually refreshing the page like I would expect. I could use a little help debugging this behavior to make sure it's working properly.

Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (d841813) 83.75% compared to head (79c6388) 83.75%.

Files Patch % Lines
Library/ViewModels/ProjectPageViewModel.swift 20.00% 3 Missing and 1 partial ⚠️
Library/ViewModels/CommentRepliesViewModel.swift 0.00% 3 Missing ⚠️
Library/ViewModels/CommentsViewModel.swift 0.00% 3 Missing ⚠️
Library/ViewModels/MessagesViewModel.swift 0.00% 3 Missing ⚠️
...overy/Controller/DiscoveryPageViewController.swift 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1885      +/-   ##
==========================================
- Coverage   83.75%   83.75%   -0.01%     
==========================================
  Files        1222     1222              
  Lines      111393   111429      +36     
  Branches    29623    29638      +15     
==========================================
+ Hits        93299    93323      +24     
- Misses      17077    17089      +12     
  Partials     1017     1017              

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


self.scheduler.advance()

self.projectsAreLoading.assertValueCount(4)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's a similar test for pullToRefresh that requires this assert to be self.projectsAreLoading.assertValueCount(6). But that seems a bit odd to me (why the extra values)? I'm not sure, then, if this is correct or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that might be because there's a loading indicator that shows for pullToRefresh (and I don't think we need to show it here). I could be wrong about that, since I didn't look thoroughly at that code. But test looks good to me!

@amy-at-kickstarter amy-at-kickstarter marked this pull request as ready for review November 15, 2023 22:11
Copy link
Contributor

@ifosli ifosli left a comment

Choose a reason for hiding this comment

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

This looks great! You're missing the ProjectPageViewModel, however. (Probably because that one currently maps to false instead of true.) Please send out a notification from there as well :)


self.scheduler.advance()

self.projectsAreLoading.assertValueCount(4)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that might be because there's a loading indicator that shows for pullToRefresh (and I don't think we need to show it here). I could be wrong about that, since I didn't look thoroughly at that code. But test looks good to me!

@amy-at-kickstarter
Copy link
Contributor Author

This looks great! You're missing the ProjectPageViewModel, however. (Probably because that one currently maps to false instead of true.) Please send out a notification from there as well :)

Whoops, good catch!

@amy-at-kickstarter amy-at-kickstarter merged commit ae907e4 into main Nov 16, 2023
7 checks passed
@amy-at-kickstarter amy-at-kickstarter deleted the feat/adyer/mbl-1029/refresh-on-block branch November 16, 2023 16:58
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