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

feat: Adds alertsConnection under Partner type to access search criteria + partner facing search crit data #5826

Merged
merged 14 commits into from
Jul 10, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions _schemaV2.graphql
Original file line number Diff line number Diff line change
Expand Up @@ -14682,6 +14682,15 @@ type Partner implements Node {
): LocationConnection
meta: PartnerMeta
name: String
partnerAlertsConnection(
after: String
before: String
first: Int
last: Int
page: Int
size: Int
totalCount: Boolean
): PartnerAlertsConnection
partnerPageEligible: Boolean
partnerType: String
profile: Profile
Expand Down Expand Up @@ -14755,6 +14764,43 @@ type PartnerAgreement {
# A partner agreement or errors object
union PartnerAgreementOrErrorsUnion = Errors | PartnerAgreement

type PartnerAlert {
artistId: String
collectorProfilesConnection(
after: String
before: String
first: Int
last: Int
totalCount: Boolean
): PartnerCollectorProfilesConnection
id: String
matchedAt: String
partnerId: String
score: String
searchCriteriaId: String
userIds: [String]
}

# A connection to a list of items.
type PartnerAlertsConnection {
# A list of edges.
edges: [PartnerAlertsEdge]
pageCursors: PageCursors!

# Information to aid in pagination.
pageInfo: PageInfo!
totalCount: Int
}

# An edge in a connection.
type PartnerAlertsEdge {
# A cursor for use in pagination
cursor: String!

# The item at the end of the edge
node: PartnerAlert
}

type PartnerArtist {
artist: Artist
artworksConnection(
Expand Down Expand Up @@ -14971,6 +15017,26 @@ enum PartnerClassification {
PRIVATE_DEALER
}

# A connection to a list of items.
type PartnerCollectorProfilesConnection {
# A list of edges.
edges: [PartnerCollectorProfilesEdge]
pageCursors: PageCursors!

# Information to aid in pagination.
pageInfo: PageInfo!
totalCount: Int
}

# An edge in a connection.
type PartnerCollectorProfilesEdge {
# A cursor for use in pagination
cursor: String!

# The item at the end of the edge
node: CollectorProfileType
}

# A connection to a list of items.
type PartnerConnection {
# A list of edges.
Expand Down
16 changes: 16 additions & 0 deletions src/lib/loaders/loaders_with_authentication/gravity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,22 @@ export default (accessToken, userID, opts) => {
{},
{ headers: true }
),
partnerSearchCriteriaLoader: gravityLoader(
(id) => `/partner/${id}/partner_search_criterias`,
{},
{ headers: true }
),
partnerCollectorProfilesLoader: gravityLoader<
any,
{ partner_id: string; user_ids: string[] }
>(
({ partner_id, user_ids }) => {
const userIdsParams = user_ids.map((id) => `user_ids[]=${id}`).join("&")
Copy link
Member

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.

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 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)

Copy link
Contributor Author

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

Copy link
Contributor

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.

partnerCollectorProfilesLoader: gravityLoader("partner_collector_profiles", {}, { headers: true })

should do it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hero 🙏🏻 !

Copy link
Contributor

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:

partnerCollectorProfilesLoader({
  partner_id: "partner-id",
  user_ids: ["a", "b", "c"]
})

(pass an object in with all the query params)

Copy link
Contributor Author

@jpotts244 jpotts244 Jul 8, 2024

Choose a reason for hiding this comment

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

Yes just like that:

const { body, headers } = await partnerCollectorProfilesLoader({
  partner_id: parent.partner_id,
  user_ids: parent.user_ids, // this looks like: [ '660ac192785b7e0025fcbcf3', '660ac191785b7e0025fcbcf1' ]
 }, gravityArgs)

but it still tries to build the query string like so:

/partner_collector_profiles?partner_id=5f80bfefe8d808000ea212c1&user_ids=660ac192785b7e0025fcbcf3,660ac191785b7e0025fcbcf1?offset=0&page=1&size=10&total_count=true

and gravity see the incoming params as ["660ac192785b7e0025fcbcf3,660ac191785b7e0025fcbcf1"]

weird right? What did I do

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! I think we are there, fat fingered something in the loader class

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return `/partner_collector_profiles?partner_id=${partner_id}&${userIdsParams}`
},
{},
{ headers: true }
),
partnerShowsLoader: gravityLoader(
(partner_id) => `partner/${partner_id}/shows`,
{},
Expand Down
63 changes: 63 additions & 0 deletions src/schema/v2/partner/__tests__/partner.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1421,6 +1421,69 @@ describe("Partner type", () => {
})
})

describe("#partnerAlertsConnection", () => {
it("returns the summary of artists with recently enabled user search criteria", async () => {
const response =
{
body: {
hits: [
{
id: '8754ff90-b020-425b-8dae-894ce5ad9d1f',
search_criteria_id: '3f980bbe-7b9b-4fa9-beb1-69d13e94fb0c',
partner_id: '5f80bfefe8d808000ea212c1',

},
{
id: 'b4adc50d-a584-4820-9140-4d49d7b6c7dc',
search_criteria_id: '614af56c-88cb-4ea7-bec5-fbdf9b67c24a',
partner_id: '5f80bfefe8d808000ea212c1',
},
],
},
headers: {
"x-total-count": 2,
},
jpotts244 marked this conversation as resolved.
Show resolved Hide resolved
}

const query = gql`
{
partner(id: "catty-partner") {
partnerAlertsConnection(first: 10) {
edges {
node {
id
}
}
}
}
}
`
const partnerSearchCriteriaLoader = () => Promise.resolve(response)
const data = await runAuthenticatedQuery(query, {
...context,
partnerSearchCriteriaLoader,
})
expect(data).toEqual({
partner: {
partnerAlertsConnection: {
edges: [
{
node: {
id: "8754ff90-b020-425b-8dae-894ce5ad9d1f",
},
},
{
node: {
id: "b4adc50d-a584-4820-9140-4d49d7b6c7dc"
}
}
],
},
},
})
})
})

describe("#alertsSummaryArtistsConnection", () => {
it("returns the summary of artists with recently enabled user search criteria", async () => {
const summaryResponse = {
Expand Down
54 changes: 54 additions & 0 deletions src/schema/v2/partner/partner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -222,6 +228,54 @@ export const PartnerType = new GraphQLObjectType<any, ResolverContext>({
})
},
},
partnerAlertsConnection: {
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably don't need the partner prefix (just alertsConnection)

type: PartnerAlertsConnectionType,
args: pageable({
page: {
type: GraphQLInt,
},
size: {
type: GraphQLInt,
},
totalCount: {
type: GraphQLBoolean,
},
Copy link
Member

@anandaroop anandaroop Jul 3, 2024

Choose a reason for hiding this comment

The 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 totalCount as an arg. I see only one case of this but many, many more where we request the total count from Gravity by default (search repo for total_count: true).

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points!
Will do some digging and clean up

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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,
Expand Down
99 changes: 99 additions & 0 deletions src/schema/v2/partner/partnerAlerts.ts
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",
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@jpotts244 jpotts244 Jul 9, 2024

Choose a reason for hiding this comment

The 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!!

Copy link
Contributor Author

@jpotts244 jpotts244 Jul 9, 2024

Choose a reason for hiding this comment

The 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
        }
      }
    }
  }
}

Copy link
Contributor Author

@jpotts244 jpotts244 Jul 9, 2024

Choose a reason for hiding this comment

The 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 AlertType as the base node and the data from partner search criteria will be on the edges in the latest commit

{
  partner(id: "commerce-test-partner") {
    slug
    name
    partnerAlertsConnection(first: 5) {
      totalCount
      edges {
        matchedAt
        score
        node {
          internalID
          colors
          additionalGeneNames
          formattedPriceRange
        }
      }
    }
  }
}

Which returns lots of nice stuff:

{
  "data": {
    "partner": {
      "slug": "commerce-test-partner",
      "name": "Commerce Test Partner",
      "partnerAlertsConnection": {
        "totalCount": 10,
        "edges": [
          {
            "partnerId": "5f80bfefe8d808000ea212c1",
            "matchedAt": null,
            "score": null,
            "node": {
              "internalID": "f7ed40ab-93d2-4e3a-82c1-025b7feb4331",
              "colors": [],
              "additionalGeneNames": [
                "Prints"
              ],
              "formattedPriceRange": null
            }
          },
          {
            "partnerId": "5f80bfefe8d808000ea212c1",
            "matchedAt": null,
            "score": null,
            "node": {
              "internalID": "2ad7213d-bbbb-4082-8eeb-e2f14d4ea64f",
              "colors": [],
              "additionalGeneNames": [
                "Painting"
              ],
              "formattedPriceRange": null
            }
          },
          {
            "partnerId": "5f80bfefe8d808000ea212c1",
            "matchedAt": null,
            "score": null,
            "node": {
              "internalID": "5ba2a417-7d7b-4728-a653-9c6b3dc96d7e",
              "colors": [],
              "additionalGeneNames": [
                "Drawing",
                "Painting",
                "Work on Paper"
              ],
              "formattedPriceRange": "$1,000 - 5,000"
            }
          },
          

Let me know if that aligns with your thinking
(I think Im going to pull out the collector profile work into a separate PR, I need to think a little bit more about its placement)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 Alert as a basic type, and we have Partner as well. On the backend, Alert is heavily based on SearchCriteria, and we now have PartnerSearchCriteria as a thing.

Whew! All that is to say, something like PartnerAlert or PartnerSearchCriteria, at least in schema terms -> to me sounded like one of those join-esque cases where edges can be a natural home and avoiding exposing (to clients), this sort-of implementation-y concept (of PartnerAlert or PartnerSearchCriteria).

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!

Copy link
Contributor

Choose a reason for hiding this comment

The 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 CollectorProfile, but some of that 'extra' partner-specific data that the Gravity endpoint exposes - if you plan on adding to it or showing it in the UI - could be along the edge. Same kind of setup with resolveNode: (resp) => resp.collector_profile and so forth

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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!

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 think I'll fast follow with CP stuff, want to play around a little more with it!

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,
}),
Copy link
Member

Choose a reason for hiding this comment

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

(We'll want to use our paginationResolver pattern like we did in the file above here)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be in latest commit

}
},
},
},
})

// export default PartnerAlertType