hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
66 stars 41 forks source link

🪣 Refactor profiles into a single user profile #5564

Closed Rotorsoft closed 2 weeks ago

Rotorsoft commented 9 months ago

Description

When adopting one profile per user, we need to review and adjust the domain model.

In the existing model (see ERD below), we allow a user to have many profiles, and each profile many addresses, enabling scenarios of data inconsistency that are difficult to enforce with db constraints.

erDiagram
    User ||--o{ Profile : has
    User {
      string email
      int id
      bool emailVerified
      bool isAdmin
      bool disableRichText
      int emailNotificationInterval 
      int selected_community_id
      date created_at
      date updated_at
    }

    Profile ||--|{ Address : has
    Profile {
      int id
      int user_id
      date created_at
      date updated_at
      string profile_name
      string email
      string website
      string bio
      string avatar_url
      string slug
      string[] socials
    }

    User ||--|{ Address : has

    Address {
      string address
      string community_id
      string verification_token
      string role
      bool is_user_default
      int id
      date verification_token_expires
      date verified
    }

Problems:

Proposed Solution:

We can simplify this model by merging User and Profile schemas into one entity. This will require changes to the models and some code refactoring (TBD)

erDiagram
    User ||--|{ Address : has

    User {
      int id
      string email
      bool email_verified
      bool is_super_admin
      int selected_community_id
      bool disable_rich_text

     JSONB profile

      date created_at
      date updated_at
    }

    Address {
      string address
      string community_id
      string verification_token
      string role
      bool is_user_default
      int id
      date verification_token_expires
      date verified
    }

Project Owner

@Rotorsoft

Engineering Requirements

Phases

Other Models

1. Dual projection

2. Query migration

Incrementally migrate queries and UI models to the new schema. This involves multiple tickets that can be combined with the tRPC API migration. The new migrated APIs will benefit from the efficiencies of the new model.

🔺 Tagging with "TO BE REMOVED" references to profile_id that should be removed once we decouple all client schemas from profile_id (during cleanup)

  • [x] #8420
  • [x] #8431
  • [x] #8442
  • [x] #8444
  • [x] #8454
  • [x] #8472
  • [x] #8502
  • [x] #8508
  • [x] #8530
  • [x] #8538

3. Command migration

Once the application’s read side is fully operating from the new model/projection, incrementally start migrating command endpoints and begin writing directly to the new model.

4. Cleanup

Rotorsoft commented 6 months ago

@jnaviask @ForestMars - Not sure about the priority here, but would like to get your thoughts before continuing with the analysis. This is basically refactoring work.

Also, where can I find some context about why we created profiles in the first place, and why we are moving to one profile/user?

ForestMars commented 4 months ago

Added cleanup tag, as this is technical debt.

kurtisassad commented 3 months ago

Just a quick note here, we have two types of emails:

User.email consists of the email that is used for SSO as well as notifications and email digests.

Profile.email is the public displayable email on the profile. It is only used for UI purposes.

If we combine these tables, we will need to distinguish between these two. Maybe having something like primary_email (user.email) and public_email (profile.email)

Rotorsoft commented 1 month ago

Planning to start working on this item next, and thinking about a JSONB user.profile column instead of multiple cols, since this is mostly for presentation purposes and would allow us to easily extend the schema without migrations. The profile can initially include:

string email
string website
string bio
string avatar_url
string slug
string[] socials

any objections @kurtisassad @timolegros ?

timolegros commented 1 month ago

Planning to start working on this item next, and thinking about a JSONB user.profile column instead of multiple cols, since this is mostly for presentation purposes and would allow us to easily extend the schema without migrations. The profile can initially include:

string email
string website
string bio
string avatar_url
string slug
string[] socials

any objections @kurtisassad @timolegros ?

Sounds good to me but @kurtisassad did the most recent work relating to profiles so leaving it to him to sign off.

Rotorsoft commented 1 month ago

Just by taking a second look, there are other associations to Profile we need to refactor:

ProfileTags - is this used? should be at user level? SsoToken - move association to user(by address id)?

@timolegros & @kurtisassad again

jnaviask commented 1 month ago

@Rotorsoft SsoToken should remain at the address level; Tags seem to be used during user onboarding? @mzparacha may know.

Rotorsoft commented 1 month ago

Yep, trying to get the first batch of changes. ready for review... there are other conflicts with email notifications that @timolegros needs to approve.. hopefully out by EOD

kurtisassad commented 1 month ago

Sorry had my email closed and didn't see this message.

To be honest I don't like JSONB, but maybe Tim has different opinions. Basically you don't get any type validation so those strings will be optionally undefined. You also can't update specific fields so lets say the client only wants to send an update to the user email, they will actually need to send all fields otherwise we will need to have custom logic on the backend for upsertion.

Lastly they are super resistant to change. The versionHistories that were JSONB had no unified schema, so when migrating them off to their separate table the migration script needed a ton of extra logic to handle the edge cases of the different types of JSONB formats.

mzparacha commented 1 month ago

Tags seem to be used during user onboarding? @mzparacha may know.

yes profile tags are for user-onboarding (user can associate multiple tags with their profiles)

cc: @jnaviask @Rotorsoft

timolegros commented 1 month ago

Sorry had my email closed and didn't see this message.

To be honest I don't like JSONB, but maybe Tim has different opinions. Basically you don't get any type validation so those strings will be optionally undefined.

We can parse with Sequelize to get types i.e. not returned as a string.

You also can't update specific fields so lets say the client only wants to send an update to the user email, they will actually need to send all fields otherwise we will need to have custom logic on the backend for upsertion.

True but fairly simple to do with raw queries.

Lastly they are super resistant to change. The versionHistories that were JSONB had no unified schema, so when migrating them off to their separate table the migration script needed a ton of extra logic to handle the edge cases of the different types of JSONB formats.

This is more so an artifact of changing schemas than it is related to JSONB. If version histories v1 were a strict model we would have had the same types of issues when migrating to the new v2 schema. In this case, we could easily change the profile JSONB schema and not face any issues unless we do a 180 and decide to separate the JSONB profile data out into a separate strict model again.

Rotorsoft commented 1 month ago

After first attempt here rotorsoft/5564-merge-user-profile with 86 changed files, we should probably reevaluate this task, considering the following affected areas:

Too big of a surface area for me to feel comfortable - Let's discuss options @jnaviask @ForestMars @timolegros

timolegros commented 1 month ago

@Rotorsoft How about transitioning incrementally? We could have the profiles table and the profiles column in the user table existing side-by-side (with unidirectional or bidirectional triggers - depending on the order of refactors) and then we can handle each area individually in separate PRs. Once everything is transitioned to the profiles column in the users table we can drop the triggers + profiles table.

Rotorsoft commented 1 month ago

@Rotorsoft How about transitioning incrementally? We could have the profiles table and the profiles column in the user table existing side-by-side (with unidirectional or bidirectional triggers - depending on the order of refactors) and then we can handle each area individually in separate PRs. Once everything is transitioned to the profiles column in the users table we can drop the triggers + profiles table.

Something like that... we can also start working with the "new" profile abstraction from the API level, starting from API (routes/controllers/middleware) into the client first, then we can replace the data schemas/models... the main theme here is user profile is an API on the user aggregate (user_id), so removing all references to profile_id from the API to the client seems also like a possible starting point.

Rotorsoft commented 1 month ago

After discussing the issue with Tim, we agree that the best strategy to tackle this ticket and the similar refactoring needed for normalizing addresses and memberships is to use the following 4-stage strategy:

  1. Projection : Build the new data model as a projection of the existing data model and write database triggers to keep it in sync with changes to the old model. This requires a migration that includes backfilling the existing data and triggers that will be discarded in the last stage.
  2. Query Migration : Incrementally migrate all queries and UI models to the new schema. This involves multiple tickets that can be combined with the tRPC API migration. The new migrated APIs will benefit from the efficiencies of the new model.
  3. Command Migration : Once the application’s read side is fully operating from the new model/projection, incrementally start migrating mutation endpoints (commands) and begin writing directly to the new model.
  4. Cleanup : Write a migration to remove the old model and triggers.

Next Steps:

Rotorsoft commented 2 weeks ago

Bucket completed by #8609