Skip to content

Conversation

bprusinowski
Copy link
Collaborator

@bprusinowski bprusinowski commented May 28, 2025

Closes #2358

This PR makes sure:

  • we only enforce uniqueness of color palettes on a user level, not globally,
  • user needs to be logged in to create / update / remove / get a color palette (was already a constraint in the UI, now also added to the backend side),
  • we remove user palettes on User deletion (cascading delete).

Also see: https://www.prisma.io/docs/orm/reference/prisma-schema-reference#unique-1


  • I added a CHANGELOG entry
  • I made a self-review of my own code

Copy link

vercel bot commented May 28, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
visualization-tool ❌ Failed (Inspect) May 28, 2025 1:17pm

@bprusinowski bprusinowski marked this pull request as ready for review May 28, 2025 12:45
@bprusinowski bprusinowski requested review from adintegra and removed request for noahonyejese May 28, 2025 12:45
@bprusinowski bprusinowski enabled auto-merge May 28, 2025 12:50
@bprusinowski bprusinowski disabled auto-merge May 28, 2025 12:52
@bprusinowski bprusinowski marked this pull request as draft May 28, 2025 12:52
@bprusinowski bprusinowski marked this pull request as ready for review May 28, 2025 13:16
Copy link
Contributor

@noahonyejese noahonyejese left a comment

Choose a reason for hiding this comment

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

LGTM overall, there still seems to be an issue on Vercel deployment but I'll approve for now. Can we now delete a user account or was this already the case in the past? 😃

@@ -29,16 +36,39 @@ export const UserController = controller({
const session = await getServerSession(req, res, nextAuthOptions);
const userId = session?.user?.id;

return await getPalettesForUser(userId);
if (!userId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could use assertion here

@bprusinowski
Copy link
Collaborator Author

Thanks @noahonyejese 🙇‍♂️ Yeah, in order to merge this PR, we'd actually first need to tackle #2360 to improve the way we handle migrations (and to fix Vercel deployment).

We can't remove accounts yet, but just to be prepared for the future 😄

@adintegra
Copy link
Contributor

adintegra commented Jun 13, 2025

Fundamentally, these changes look good to me. However, the client has recently decided that unique palette names (globally) is useful to them. It may therefore make sense to drop the uniqueness constraint changes here but keep the other enhancements, i.e.

  • user needs to be logged in to create / update / remove / get a color palette (was already a constraint in the UI, now also added to the backend side),
  • we remove user palettes on User deletion (cascading delete).

For added context, there are ongoing conversations outside this thread regarding the overall streamlining of the DB design, enhancing it to accommodate palette sharing and potentially moving to the proper use of prisma migrations (i.e. moving from prisma db push to prisma migrate dev + prisma migrate deploy) – see #2360.

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.

Color Palette Names are unique across the application
3 participants