-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat: Adds alertsConnection under Partner type to access search criteria + partner facing search crit data #5826
Changes from 6 commits
2a95760
de901c8
4ac3f12
37143cf
73c323f
322eb12
1af8422
0b38045
0dcc65c
64df5b8
32f6a40
b17b402
cbfb612
6d7638c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,6 +51,7 @@ import { | |
ArtworkVisibility, | ||
ArtworkVisibilityEnumValues, | ||
} from "schema/v2/artwork/artworkVisibility" | ||
import { PartnerAlertType } from "./partnerAlerts" | ||
|
||
const isFairOrganizer = (type) => type === "FairOrganizer" | ||
const isGallery = (type) => type === "PartnerGallery" | ||
|
@@ -154,6 +155,11 @@ export const PartnerType = new GraphQLObjectType<any, ResolverContext>({ | |
nodeType: ArtistType, | ||
}).connectionType | ||
|
||
const PartnerAlertsConnectionType = connectionWithCursorInfo({ | ||
name: "PartnerAlerts", | ||
nodeType: PartnerAlertType, | ||
}).connectionType | ||
|
||
return { | ||
...SlugAndInternalIDFields, | ||
cached, | ||
|
@@ -222,6 +228,54 @@ export const PartnerType = new GraphQLObjectType<any, ResolverContext>({ | |
}) | ||
}, | ||
}, | ||
partnerAlertsConnection: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you probably don't need the |
||
type: PartnerAlertsConnectionType, | ||
args: pageable({ | ||
page: { | ||
type: GraphQLInt, | ||
}, | ||
size: { | ||
type: GraphQLInt, | ||
}, | ||
totalCount: { | ||
type: GraphQLBoolean, | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thought: It strikes me as un-idiomatic for an MP field to take I think some pagination logic even relies on that total count being present, so perhaps better to include it by default? Furthermore… I believe this query is ultimately fulfilled by ES, yes? If so I think that total count is always computed and doesn't even make sense as an arg (in which case this arg over in the upstream Gravity endpoint might be totally superfluous). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good points! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be addressed 🙏🏻 |
||
}), | ||
resolve: async ({ _id }, args, { partnerSearchCriteriaLoader }) => { | ||
if (!partnerSearchCriteriaLoader) return null | ||
const { page, size, offset } = convertConnectionArgsToGravityArgs( | ||
args | ||
) | ||
|
||
type GravityArgs = { | ||
page: number | ||
size: number | ||
total_count?: number | ||
} | ||
|
||
const gravityArgs: GravityArgs = { | ||
page, | ||
size, | ||
total_count: args.totalCount, | ||
} | ||
|
||
const { body, headers } = await partnerSearchCriteriaLoader?.( | ||
_id, | ||
gravityArgs | ||
) | ||
const totalCount = parseInt(headers["x-total-count"] || "0", 10) | ||
|
||
return paginationResolver({ | ||
totalCount, | ||
offset, | ||
page, | ||
size, | ||
body: body.hits, | ||
args, | ||
resolveNode: (node) => node, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. prob don't need this line? |
||
}) | ||
}, | ||
}, | ||
articlesConnection: { | ||
description: "A connection of articles related to a partner.", | ||
type: articleConnection.connectionType, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,99 @@ | ||
import { pageable } from "relay-cursor-paging" | ||
import { | ||
GraphQLString, | ||
GraphQLObjectType, | ||
GraphQLList, | ||
GraphQLBoolean, | ||
} from "graphql" | ||
import { | ||
connectionWithCursorInfo, | ||
createPageCursors, | ||
} from "schema/v2/fields/pagination" | ||
import { connectionFromArraySlice } from "graphql-relay" | ||
import { convertConnectionArgsToGravityArgs } from "lib/helpers" | ||
import { CollectorProfileType } from "schema/v2/CollectorProfile/collectorProfile" | ||
|
||
const PartnerCollectorProfilesConnectionType = connectionWithCursorInfo({ | ||
name: "PartnerCollectorProfiles", | ||
nodeType: CollectorProfileType, | ||
}).connectionType | ||
|
||
export const PartnerAlertType = new GraphQLObjectType({ | ||
name: "PartnerAlert", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New concept! We have an 'alert' abstraction already -> so what is a 'partner alert'? If it's all just alerts all the way down at the end of the day - this adds a new abstraction that might not be needed (better to use 'Alert'). An alternate schema possibly might be a connection between a partner and alerts (so a connection with the node being an alert), with some of the partner-specific stuff (like 'score') on the edge. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that 'partner alert' and 'alert' share many similarities, and good points there, particularly in terms of metadata like attribution class and price range. However, there are some differences that I believe necessitate a distinction, such as handling the scoring on individual nodes (? might just be a noob here but thinking things specific to the node only). (This involves determining which alert is the best, whether the partner has already satisfied this alert by uploading an artwork, and soon, sharing an artwork with users subscribed to the alert. etc etc) Your suggestion of combining the Alert type with some partner-specific fields could indeed address both aspects effectively. I'll experiment with this idea and sketch out a schema that incorporates these considerations!! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I think I understand now, I was thinking attributes on the edge would be able the "collection" of nodes but its more about the relationship between partner <> alert and then things like collector profile could live on the edge as its "partner-y" Pseudo-thinking: {
"data": {
"partner": {
"id": "1",
"name": "Partner A",
"alerts": {
"edges": [
{
"node": {
"id": "im-an-alert-1",
"createdAt": "2024-07-09T12:00:00Z"
},
"score": 85,
"cursor": "cursor1"
},
{
"node": {
"id": "im-an-alert-2",
"createdAt": "2024-07-09T13:00:00Z"
},
"score": 90,
"cursor": "cursor2"
}
],
"pageInfo": {
"endCursor": "cursor2",
"hasNextPage": true
}
}
}
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok @mzikherman Ive noted that I owe you some bagels again in my TODOs. I've adjusted this to return
Which returns lots of nice stuff:
Let me know if that aligns with your thinking There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes! I suggested it a bit hastily (potentially - didn't necessarily mean for you to definitely go that route - but it seems like it was fruitful) - and more automatically. What I mean is that - a connection with data on the edges is a nice setup for join models, and 'synthetic'-ish models of that kind. So if we have a concept of 'type A', and 'type B', if someone adds 'type AB' or 'type BA' - that's an automatic signal to me to suggest A -> B (and put stuff on the edge), or B -> A (and put stuff on the edge). So here, we have Whew! All that is to say, something like However - of course there's no real right or wrong answer here - there's some science but there's an art as well - if this feels like a nice schema design lets do it! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, potentially keep this in mind for exposing of collector profiles. You'd want the node to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ yes thanks for the deeper dive, I think I agree! This feels better once in the fingers and reviewing this article you've shared in the past! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'll fast follow with |
||
fields: { | ||
id: { type: GraphQLString }, | ||
searchCriteriaId: { | ||
type: GraphQLString, | ||
resolve: ({ search_criteria_id }) => search_criteria_id, | ||
}, | ||
partnerId: { | ||
type: GraphQLString, | ||
resolve: ({ partner_id }) => partner_id, | ||
}, | ||
score: { type: GraphQLString }, | ||
matchedAt: { | ||
type: GraphQLString, | ||
resolve: ({ matched_at }) => matched_at, | ||
}, | ||
userIds: { | ||
type: new GraphQLList(GraphQLString), | ||
resolve: ({ user_ids }) => user_ids, | ||
}, | ||
artistId: { | ||
type: GraphQLString, | ||
resolve: ({ artist_id }) => artist_id, | ||
}, | ||
collectorProfilesConnection: { | ||
type: PartnerCollectorProfilesConnectionType, | ||
args: pageable({ | ||
totalCount: { | ||
type: GraphQLBoolean, | ||
}, | ||
}), | ||
resolve: async (parent, args, { partnerCollectorProfilesLoader }) => { | ||
if (!partnerCollectorProfilesLoader) return null | ||
|
||
const { page, size, offset } = convertConnectionArgsToGravityArgs(args) | ||
|
||
console.log("inside seconds api call") | ||
type GravityArgs = { | ||
page: number | ||
size: number | ||
partner_id: string | ||
user_ids: string[] | ||
total_count?: number | ||
} | ||
|
||
const gravityArgs: GravityArgs = { | ||
page, | ||
size, | ||
total_count: args.totalCount, | ||
partner_id: parent.partner_id, | ||
user_ids: parent.user_ids, | ||
} | ||
|
||
const data = await partnerCollectorProfilesLoader(gravityArgs) | ||
|
||
console.log("data", data) | ||
|
||
const collectorProfiles = data.body.flatMap((item) => | ||
item.collector_profile ? [item.collector_profile].flat() : [] | ||
) | ||
|
||
return { | ||
totalCount: collectorProfiles.length, | ||
pageCursors: createPageCursors( | ||
{ ...args, page, size }, | ||
collectorProfiles.length | ||
), | ||
...connectionFromArraySlice(collectorProfiles, args, { | ||
arrayLength: collectorProfiles.length, | ||
sliceStart: offset, | ||
}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (We'll want to use our There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On it! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be in latest commit |
||
} | ||
}, | ||
}, | ||
}, | ||
}) | ||
|
||
// export default PartnerAlertType |
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.
not-blocking: do we need to worry about the max length of a query string? not sure what is the max number of collector profiles we'll want to show at one time for a given alert, but I can see this exceeding limits for alerts with lots of collectors.
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.
I believe we do, can play around with edge cases here (or have a default limit of user ids here or at gravity's level for protection)
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.
Added a max length to the API endpoint but ill see about adding some protecting here too
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.
You shouldn't have to manually do any of this for query params in MP.
should do it
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.
Hero 🙏🏻 !
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.
How are you invoking the loader? With the simplified form I wrote above, you should be calling it:
(pass an object in with all the query params)
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.
Yes just like that:
but it still tries to build the query string like so:
and gravity see the incoming params as
["660ac192785b7e0025fcbcf3,660ac191785b7e0025fcbcf1"]
weird right? What did I do
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.
Ah! I think we are there, fat fingered something in the loader class
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.
I don't think you're invoking it properly. In your quoted example, there needs to be one argument (an object) passed to the loader. What is
gravityArgs
?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.
(Fixed that as well)