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

fix: Remove manual population of creator ID #3884

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Oct 31, 2024

What's the problem?

  • The /flows/:flowId/copy endpoint is broken - throwing a permission error due to a missing field creator_id
  • All roles which write to this table have write permissions for this column

What's the cause?

When we added a column preset for creator_id (#3864) it removed the columns from the insert schema for the flows table.

The respective fields will also be removed from the generated GraphQL schema for that role.

Source: https://hasura.io/docs/2.0/auth/authorization/permissions/column-presets/

image

What's the solution?

  • Let Hasura handle this and populate it from the JWT, remove our custom code for fetch the user ID from the JWT
  • It's actually a pretty nice solution / pattern imho!
  • I've checked for other references to creator_id and creatorId - this is the only one in this repo and planx-core

@DafyddLlyr DafyddLlyr marked this pull request as draft October 31, 2024 09:13
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 want to overcomplicate this but, would this be a good time to switch this to the CreateFlow mutation from planx-core so it matches the editor? @DafyddLlyr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, makes sense to standardise this I think and it could have avoided this issue.

As this is currently blocking work, I'd vote for moving this forward just now and getting to prod and picking up this refactor as a follow up?

Copy link

github-actions bot commented Oct 31, 2024

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr requested a review from a team October 31, 2024 09:31
@DafyddLlyr DafyddLlyr marked this pull request as ready for review October 31, 2024 09:31
Copy link
Contributor

@RODO94 RODO94 left a comment

Choose a reason for hiding this comment

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

Fixes the bug for me, thanks for picking this up!

@DafyddLlyr DafyddLlyr merged commit 447e33e into main Oct 31, 2024
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/remove-manual-population-of-creator-id branch October 31, 2024 09:48
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.

2 participants