Skip to content

Add a new query to fetch the PLOT-related date in the Manage Pledge flow#2495

Merged
jovaniks merged 8 commits into
mainfrom
jluna/query-fetch-project-plot-data
Jun 30, 2025
Merged

Add a new query to fetch the PLOT-related date in the Manage Pledge flow#2495
jovaniks merged 8 commits into
mainfrom
jluna/query-fetch-project-plot-data

Conversation

@jovaniks

@jovaniks jovaniks commented Jun 25, 2025

Copy link
Copy Markdown
Contributor

📲 What

Adds a new GraphQL query called FetchProjectPledgeOverTimeData to fetch PLOT-related project information from the backend.

🤔 Why

The Manage Pledge flow for PLOT-enabled projects requires certain project-level PLOT fields that are currently only available via GraphQL.

Since updating the existing V1 endpoint is not feasible at this time, this new query will allow us to fetch just the necessary PLOT fields while keeping the rest of the flow intact.

This query enables the function fetchProjectPledgeOverTimeData to retrieve the required information when editing PLOT-enabled pledges.

🛠 How

  • Introduced a new query: FetchProjectPledgeOverTimeData
query FetchProjectPledgeOverTimeData($projectId: Int!) {
  project(pid: $projectId) {
    __typename
    isPledgeOverTimeAllowed
    pledgeOverTimeCollectionPlanChargeExplanation
    pledgeOverTimeCollectionPlanChargedAsNPayments
    pledgeOverTimeCollectionPlanShortPitch
    pledgeOverTimeMinimumExplanation
  }
}

👀 See

Simulator Screen Recording - iPhone 16 Pro - 2025-06-25 at 08 23 36

⏰ TODO

  • Ideally move forward to swift the fetch project from V1 to GraphQL

@jovaniks jovaniks requested a review from scottkicks June 25, 2025 15:38
Comment thread KsApi/ServiceType.swift Outdated
Comment thread Library/ViewModels/ManagePledgeViewModel.swift Outdated
Comment thread KsApi/models/ProjectPLOTDataEnvelope.swift Outdated
@jovaniks jovaniks changed the title Add FetchProjectPLOTData query to get the PLOT related info Add a new query to fetch the PLOT-related date in the Manage Pledge flow Jun 25, 2025
@jovaniks jovaniks marked this pull request as ready for review June 25, 2025 17:36

@amy-at-kickstarter amy-at-kickstarter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but requesting a small change in nullability!

return SignalProducer(error: .couldNotParseJSON)
}

let envelope = ProjectPledgeOverTimeDataEnvelope(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like GraphQL defines all of these as optional. These envelope values should be String? instead of String, so then you can get rid of these empty string placeholder values.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I initially had them as optional but changed them to non-optional to align with the Project model. No big deal, can revert if needed.

@amy-at-kickstarter amy-at-kickstarter Jun 25, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Mostly I'm always a bit suspicious of passing around empty placeholder strings! I find nil clearer.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great, I'll go ahead and make the change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done! PLOT-related fields are now nullable both in ProjectPledgeOverTimeDataEnvelope and in the Project model.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

intentClientSecret: input.setupIntentClientSecret,
applePay: GraphAPI.ApplePayInput.from(input.applePay)
applePay: GraphAPI.ApplePayInput.from(input.applePay),
incremental: input.incremental

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adds the missing incremental field to ensure we can pass this value through the UpdateBacking mutation.

@jovaniks jovaniks force-pushed the jluna/query-fetch-project-plot-data branch from 83100d2 to fc8f47a Compare June 26, 2025 01:52

@amy-at-kickstarter amy-at-kickstarter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Largely LGTM but I have a specific refactoring or follow-up request.

return SignalProducer(value: project)
}

return fetchProjectPledgeOverTimeData(project: project)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly, this will fire three requests in a row for PLOT projects: the first V1 request to fetch the project, then a second GraphQL request to fetch the rewards, and a third GraphQL request to fetch the PLOT data. Chaining together requests like this is ultimately going to lead to a much slower user experience, since it's three network round trips.

Since fetchProjectRewards is a GraphQL request already, could you fold the pledgeOverTime fragment into the second request?

I don't want to block this PR on this, so perhaps you can file a follow-up ticket for this issue. I assume PLOT pledges are a minority of pledges, but I do think this is worth fixing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good call!!! I will go for it on Monday in a follow-up ticket!

@jovaniks jovaniks merged commit c57b168 into main Jun 30, 2025
5 checks passed
@jovaniks jovaniks deleted the jluna/query-fetch-project-plot-data branch June 30, 2025 16:20
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.

3 participants