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-1931] Update FetchUserSetup to include ppoHasAction in fragment #2254

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

stevestreza-ksr
Copy link
Contributor

📲 What

This PR re-adds ppoHasAction to the user loading query, this time as part of FetchUserSetup. This restores the action loading as part of setup, but doesn't break logged-out users loading projects.

🤔 Why

This functionality was reverted for breaking loading projects for logged-out users in #2218.

🛠 How

First, the original disabling PR was reverted to restore the functionality. Then, I created a new fragment, PPOUserSetupFragment, which loads the ppoHasAction field. Then, the field was removed from the old query and the new fragment was added to the FetchUserSetup query.

✅ Acceptance criteria

  • When the feature flag is enabled, the ppoHasAction will be reflected in the badge on the activity tab
  • When logged out, tapping a project will not cause an error to appear in the "back this project" bar

AppEnvironment.replaceCurrentEnvironment(
currentUser: currentUser,
currentUserEmail: email,
currentUserServerFeatures: features
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a strong reason to keep ppoHasAction on AppEnvironment.current.user, instead of pulling it out like AppEnvironment.current.userHasPPOAction?

Mutating an object that was returned directly from an API request like currentUser causes me to raise an eyebrow - I don't see it causing any immediate problems, but it's a precedent that makes me nervous.

That said, the other pattern (currentUserEmail) isn't beautiful either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point I think that is just a carryover from originally building this on User. I'll move it out to this pattern for now.

We'll probably want to rethink "extra user stuff that shouldn't be on User" at some point.

Copy link
Contributor

@amy-at-kickstarter amy-at-kickstarter Jan 23, 2025

Choose a reason for hiding this comment

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

Agreed!

Completely out of scope here, but "extra user stuff that shouldn't be on User" touches on two interesting pieces of tech debt:

  1. Everything we've stapled on to User comes from GraphQL. Would making our session login/logout a GraphQL call with a nicely apportioned fragment solve this problem?
  2. Immutable server objects are one of the annoying things preventing us from getting rid of Prelude, since we do this Project.template |> Project.lens.foo .~ bar all the time in our tests. There's an open question for what we want to do to solve that problem - do we make all our model objects mutable, or implement some other pattern like a builder (User.builder().ppoHasAction(true).email("[email protected]")...) or mock model objects for testing (MockApiObject<User> foo; foo.ppoHasAction = true)?

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 the problem for 1 is twofold, one is how we load the query now (which we do through FetchUserSetup and fragments now), and the other is the storage side on AppEnvironment (this is why we're overloading with multiple properties right now).

We did discuss a bit about immutable objects a couple months ago. One tool we have now that we didn't have before is macros, so we might be able to build something that generates builders for those.

@stevestreza-ksr stevestreza-ksr force-pushed the stevestreza/ppo/ppoHasAction-user-fragment branch 2 times, most recently from 27bc87e to 93eab04 Compare January 23, 2025 00:25
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Maybe we can discuss how we might address Amy's out of scope points in an iOS sync!

@stevestreza-ksr stevestreza-ksr force-pushed the stevestreza/ppo/ppoHasAction-user-fragment branch from 93eab04 to 5ff6d82 Compare January 23, 2025 21:57
@stevestreza-ksr stevestreza-ksr merged commit 87ddef8 into main Jan 23, 2025
5 checks passed
@stevestreza-ksr stevestreza-ksr deleted the stevestreza/ppo/ppoHasAction-user-fragment branch January 23, 2025 22:40
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