-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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-1022: Add blocking mutation to iOS client #1890
Conversation
@@ -6716,7 +6841,7 @@ public enum GraphAPI { | |||
} | |||
|
|||
public struct Node: GraphQLSelectionSet { | |||
public static let possibleTypes: [String] = ["Backing", "Reward", "Photo", "RewardItem", "Project", "Comment", "User", "Address", "Conversation", "Message", "CuratedPage", "Location", "Organization", "UserUrl", "Category", "AiDisclosure", "BusinessAddress", "ProjectFeaturedImage", "Flagging", "Video", "VideoTrack", "VideoTrackCue", "ProjectProfile", "AttachedAudio", "AttachedVideo", "Tag", "CreatorInterview", "InterviewAnswer", "InterviewQuestion", "CreatorPrompt", "FreeformPost", "ShippingRule", "Checkout", "Order", "OrderAddress", "Survey"] | |||
public static let possibleTypes: [String] = ["Backing", "Reward", "Photo", "RewardItem", "Project", "Comment", "User", "Address", "Conversation", "Message", "CuratedPage", "Location", "Organization", "UserUrl", "Category", "AiDisclosure", "BusinessAddress", "Flagging", "Video", "VideoTrack", "VideoTrackCue", "ProjectProfile", "AttachedAudio", "AttachedVideo", "Tag", "CreatorInterview", "InterviewAnswer", "InterviewQuestion", "CreatorPrompt", "FreeformPost", "ShippingRule", "Checkout", "Order", "OrderAddress", "Survey"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were a few other changes in the GraphQL schema since it was last updated, so I assume these are part of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One point worth noting - right now we only update the schema when we need to update it on the client. But we should really be updating it any time changes to the web schema are pushed out - otherwise, we risk getting out of sync, and (say) using a feature that no longer actually exists on the web.
Obviously I'd hope that someone working on the web side would point this out to us before merging that change! But nice to enforce the same thing through code, not just socially.
return GraphQL.shared.client | ||
.perform(mutation: GraphAPI.BlockUserMutation(input: GraphAPI.BlockUserInput(blockUserId: userId))) | ||
.flatMap { _ in | ||
SignalProducer(value: EmptyResponseEnvelope()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So BlockUser
can return two fields: success
and currentUser { blockedUsers }
. However, based on my current understanding of the client designs, we don't need to read either of these fields: we'll fetch isBlocked
directly on other User
objects.
Is that correct? Based on that assumption I figured I'd take the simplest route out and return an EmptyResponseEnvelope
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep exactly. We only need to know if it was successful right now.
KsApi/Service.swift
Outdated
public func blockUser(userId: String) | ||
-> SignalProducer<EmptyResponseEnvelope, ErrorEnvelope> { | ||
return GraphQL.shared.client | ||
.perform(mutation: GraphAPI.BlockUserMutation(input: GraphAPI.BlockUserInput(blockUserId: userId))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphAPI.BlockUserMutation
wants us to pass it a GraphQLID
type, which can also be a String
. It's not obvious to me if we'll need to turn the ID into a base64-encoded value, like a proper GraphQL identifier, or if it will be happy taking a plain, string-based user ID like "12345"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the same "adapter" pattern that we're using elsewhere here. We can create an extension on GraphAPI.BlockUserInput
for creating the input type. Extracting this allows us to test it more easily. See GraphAPI.WatchProjectInput.from(_ input:...
for an example
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1890 +/- ##
==========================================
- Coverage 87.76% 83.72% -4.04%
==========================================
Files 858 1225 +367
Lines 77001 111519 +34518
Branches 20418 29673 +9255
==========================================
+ Hits 67583 93374 +25791
- Misses 8677 17125 +8448
- Partials 741 1020 +279 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice first run through! There's just a couple of things we need to add to stick to the existing pattern and keep things well tested. I added inline comments and you can use the linked PR below as a guide.
Let's create a BlockUserInput
type to inject into our Service method as well as a .from(_ input...)
helper extension to make it easier on ourselves to bundle up the expected properties once we implement this. Check out this PR out as a reference
Oh and we can add some tests around the input and extension as well 😄
KsApi/Service.swift
Outdated
public func blockUser(userId: String) | ||
-> SignalProducer<EmptyResponseEnvelope, ErrorEnvelope> { | ||
return GraphQL.shared.client | ||
.perform(mutation: GraphAPI.BlockUserMutation(input: GraphAPI.BlockUserInput(blockUserId: userId))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's follow the same "adapter" pattern that we're using elsewhere here. We can create an extension on GraphAPI.BlockUserInput
for creating the input type. Extracting this allows us to test it more easily. See GraphAPI.WatchProjectInput.from(_ input:...
for an example
return GraphQL.shared.client | ||
.perform(mutation: GraphAPI.BlockUserMutation(input: GraphAPI.BlockUserInput(blockUserId: userId))) | ||
.flatMap { _ in | ||
SignalProducer(value: EmptyResponseEnvelope()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep exactly. We only need to know if it was successful right now.
KsApi/ServiceType.swift
Outdated
@@ -380,6 +380,8 @@ public protocol ServiceType { | |||
|
|||
func fetchBackedProjects(cursor: String?, limit: Int?) | |||
-> SignalProducer<FetchProjectsEnvelope, ErrorEnvelope> | |||
|
|||
func blockUser(userId: String) -> SignalProducer<EmptyResponseEnvelope, ErrorEnvelope> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this will be func blockUser(input: BlockUserInput) -> SignalProducer<EmptyResponseEnvelope, ErrorEnvelope>
Generated by 🚫 Danger |
@@ -0,0 +1,5 @@ | |||
import Foundation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@scottkicks Can you explain a little more about this input adapter pattern? Happy to follow it for now, but at least for a very teeny input like this, it doesn't seem obviously beneficial. I'm curious to hear more about why we've been using this pattern instead of using the GraphAPI.XYZInput
objects directly.
fea8e73
to
01c9bd4
Compare
01c9bd4
to
a730d13
Compare
@amy-at-kickstarter Until we can say this current mutation works and is testable as expected, we might want to hold off on merging this. In case Santiago has to make further changes. ref: https://kickstarter.slack.com/archives/C05HX9GS0CD/p1701219627599909?thread_ts=1701184665.560489&cid=C05HX9GS0CD |
📲 What
Add
BlockUser
mutation to the app.🤔 Why
So we can implement blocking in the UI!