joemasilotti / railsdevs.com

The reverse job board for Ruby on Rails developers.
https://railsdevs.com
MIT License
774 stars 260 forks source link

Account vs. User #237

Closed joemasilotti closed 1 year ago

joemasilotti commented 2 years ago

Well, it was bound to come eventually! A potential customer expressed interest in needing multiple users per paid account. I'm still working through how this will all work, so this comment will serve as a living doc of how things will be handled.

Proposal

  1. Create an Account model with an owner_id column, User
  2. Add account_id to User model, belongs_to: :account
  3. Migrate user_id on Business and Developer to account_id
    • Right now each account will only have the one user, so create a single account per user
    • Set the owner attribute on the account to the user
  4. Migrate all related queries from current_user to current_user.account
    • Create abstraction, #current_account, in ApplicationHelper or ApplicationController
    • Pundit policies, PaywalledComponent, DeveloperPrimaryActionComponent
  5. Build out invitation flow for the account owner to invite a non-owner user associated with the account
    • To start, they have full permissions with the exception of inviting someone else

Open questions

  1. How does the noticed integration change?
    • The account should now be notified, so maybe all users associated with the account get the notification.
    • But if person A reads one, does person B's notification (from the same account) also get read?
  2. How does the Pay integration change?
    • pay_customer expects an email and name, but this will live on Account now. How can the owner's details be passed to/from Pay?
  3. Having an additional query (user.account.object) every time we need data feels like it could be bad, but I might be pre-optimizing.

Perhaps this ~30 line multi-tenant strategy can help?

subdigital commented 2 years ago

Having done this before, I can tell you it's good that you're doing it now rather than later :)

You hit most of the points I'd suggest. A couple of other things to consider:

tommasongr commented 2 years ago

I'm not an expert but for an app I'm working on I used the following configuration:

Basically Accounts and Users are associated through a Membership model. This way a User can have multiple accounts and an account can have multiple owners or other roles.

You still have to ensure that the last owner on the account is not deleted before the account itself but apart from that this setup should cover all the good points from @subdigital (I guess).

Account

class Account < ApplicationRecord
  has_many :memberships, dependent: :destroy
  has_many :users, through: :memberships

  accepts_nested_attributes_for :memberships
end

User

class User < ApplicationRecord
  has_many :memberships, dependent: :destroy
  has_many :accounts, through: :memberships
end

Membership

class Membership < ApplicationRecord
  belongs_to :account
  belongs_to :user

  accepts_nested_attributes_for :account

  enum :role, { owner: 0, admin: 1, user: 2 }, suffix: true, default: :user
end

Sign up

When a user signs up a default account is created through a new membership

@user.memberships.build(role: :owner).build_account(name: @user.name)

Perhaps this ~30 line multi-tenant strategy can help?

For interacting I'm using this exact strategy with some minor adjustments. It's working great!


Having an additional query (user.account.object) every time we need data feels like it could be bad, but I might be pre-optimizing.

This can be solved with Current. You check if the user can access the account once and save it in Current.account.

Tonksthebear commented 2 years ago

This is slightly off-topic. I'm not sure if this is helpful, but I've had a lot of success using data_migrate when needing to add/change/remove data in rows. It seems like closing this issue will require changing some data, so I figured I'd toss it out there as a potential nice solution. I'm not sure how larger rails apps handle these situations, but, in my research, there wasn't a clear happy path.

joemasilotti commented 2 years ago

Thanks for the feedback and links everyone!

At this point I'm leaning towards building out all of the modeling for Accounts but hiding everything from the user. Meaning, when you sign up you are assigned an account and all permissions and subscriptions are done via accounts. I can launch that and ensure nothing broke.

IMG_3958

Then, once that is live, I can start layering in the UI needed to flesh out the actual feature. Most of what @subdigital is talking about, but limited to specific use-cases for businesses. (I don't ever see a developer needing to manage multiple profiles.)


@Tonksthebear, OK humor me. What's so bad about running data migrations inside the database migration? I've done it before without issue but I've heard it's not a great practice. Would love to know more and that gem looks like a great solution.

joemasilotti commented 2 years ago

More concretely, a series of PRs could be as follows:

  1. Scheme and data migrations only - no UI changes
    1. Add Account
    2. Move Developer and Business from User to Account
    3. Move Conversation and Message from User to Account
    4. Move pay_customer from User to Account
    5. Send user-based notifications to all users of the account
    6. Newly created User records get their own Account
    7. Add Account when creating each Notification
    8. Backfill:
      • user_id -> account_id on Developer, Business, Conversation, and Pay::Customer
      • Every User their own Account
      • Add the user's Account to every Notification
  2. Introduce account switching to/from Business and Developer
    1. current_account helper
    2. Save current account in session
    3. Switch to account via user dropdown
    4. Show all user's notifications
    5. Switch to notification's account when viewing it (accounts for coming in from an email, too)
  3. Clean up Conversation and Message
    1. Display Business name in top to developer
    2. Always display user's real name in the message (accounts for multiple users per account)
    3. Rework logic of "who sent this" and "who received it"
  4. Add /businesses/:id UI for account management
    1. Invite a new user to their account
    2. Remove an existing user from their account (but not themself)
    3. Non-owners cannot invite other users to that account

Handle the rest via the console!

Tonksthebear commented 2 years ago

@Tonksthebear, OK humor me. What's so bad about running data migrations inside the database migration? I've done it before without issue but I've heard it's not a great practice. Would love to know more and that gem looks like a great solution.

I'm not sure if I can give good examples of how it is BAD per se. I like keeping them separated because I like the logical separation of schema modifications and data modifications. I also thought it would be risky to have a migration file potentially manipulate data based on old model validations (could prevent a successful migration on a new environment). I like the guarantee that schema migrations will ALWAYS work on a new environment, whereas issues with the data migrations could be sorted out separately. I might be overlooking an obvious way to handle that scenario though.

I also like intentionally making data changes with a separate command. Schema changes can obviously also affect data, but I know the really serious changes will require rails data:migrate.

Let me know if you have any other questions. I've only ever worked solo, so I don't have knowledge other people might have on the subject, especially with larger databases. This is just the logic I ended up using 👍

joemasilotti commented 2 years ago

Makes sense to me – thanks for the brain dump!

joemasilotti commented 2 years ago

@jayeclark, can you please comment on this issue so I can assign it you?

jayeclark commented 2 years ago

👋

jayeclark commented 2 years ago

Dropping some thoughts here now that I've started messing with actual code... I've tried (very loosely) sketching out my initial approach a bit below.

In the initial set of work, a Business will initially still belong to a User and a User will only be able to own one Business. However, there will be an additional class UserRoleAtBusiness linking users to an existing Business and granting them permissions to initiate conversations on the Business' behalf, to invite other users or converse with candidates. Whether or not you can speak on behalf of a business will be controlled by this relationship. There will be a cap on user roles per Business based on subscription tier, and a user can only have one role per Business. If you are a user associated with a business, but are not the owner, you can view the business profile (from the dropdown on your avatar) but cannot edit it.

Current

classDiagram
User -->DeveloperProfile:has one<br>
User -->Subscription:has one active?
User-->Business: has one<br>
Subscription-->Business:Determines<br>permissions for
class Business{
user_id
company
website
bio
developer_notifications
contact_name
contact_role
}
class DeveloperProfile{
user_id
name
hero
bio
website
linkedin
github
twitter
search_status
role_type
role_level
available_on
}

Proposed

classDiagram
User -->UserRoleAtBusiness:has many
User -->DeveloperProfile:has one
User -->Subscription:has one active?
User-->Business: has one
UserRoleAtBusiness <-- Business: has many
Subscription-->Business:Determines<br>permissions for
class UserRoleAtBusiness{
role_type
entity_id
user_id
contact_name
contact_role
}
class Business{
user_id
company
website
bio
developer_notifications
send_notifications_to
}
class DeveloperProfile{
user_id
name
hero
bio
website
linkedin
github
twitter
search_status
role_type?
role_level?
available_on?
}

Suggested Initial Plan

Backend

  1. Create a UserRoleAtBusiness model (default role_type = member) and create owner records for all existing businesses
  2. Change conversation model & controller to track the initiating user as well as the participating business & developer, and address what happens to a conversation when a business user is no longer associated with that business (dropbox is a pretty good model here I think - conversation ownership reverts to the business owner)
  3. Update notifications along the same lines

    Invisible front-end

  4. Migrate front end from using Business contact_name and contact_role to using UserRoleAtBusiness contact_name and contact_role
  5. Update Pundit policies, PaywalledComponent, BusinessPrimaryActionComponent (Non-owner users can see but not edit the business profile), ApplicationController

Visible front-end

  1. Add feature flag for UserRoleAtBusiness related frontend additions
  2. Build out invitation flow for a Business owner to invite another user to become associated with the Business
  3. Build out flow for user to voluntarily leave a Business and for owner to remove a user from an Business
  4. Update user_component links to list all businesses for which the user has an owner role OR for which the user has a role & the business has an active subscription.
  5. Update 'Start A Conversation' page to allow user to initiate a conversation if they have a business role but are not a business owner, and to be able to switch among Businesses if they have a role at more than one
  6. Update conversations UI (overall list and individual view) to make the context clearer (i.e. is this a conversation where I'm trying to hire someone, or where they're trying to hire me?)
  7. Remove feature flag

Questions

  1. Should business owners be able to see ALL conversations taking place on behalf of their business?
  2. Should business owners be able to take over a conversation?
  3. Any thoughts on the limit to the number of 'seats' per subscription type?
  4. Avatars.& Names. It seems 'off' to me that a user can have one name as a Developer and another contact_name associated with a Business, and that their avatar will be different when they're having conversations as one or the other. Doesn't need to be addressed in the initial MVP but I really liked the line of reasoning that was happening on Twitter, where all users would have a basic profile (avatar, bio, maybe some social links) and then the concept of 'Developer' becomes more of a 'DeveloperProfile' class that extends Profile and determines job board placement. That way, when a person reaches out as a user on behalf of a business, the job seeker would see both their personal info and the details of the company they're representing.
joemasilotti commented 2 years ago

This looks like a great start, thanks for kicking it off!

There's one thing that doesn't seem to be addressed. If a business account owns a conversation, what happens when a second user (associated with said business) tries to respond? Should it look like it came from the original business sender? Or do we need a way to differentiate in the UI who sent the business (like Airbnb).

Answers

  1. Yes, if you have access to a business then you can see all of the conversations said business is involved in.
  2. I think this kind of hits on the above. If we implement >2 participants in a conversation then no.
  3. I don't think we need to worry about seats just yet. We can skip this.
  4. Agreed. And I think this might hit on more of the core of the issue and snowball a tad!

Continuing from 4. above, I'm having those scary second thoughts of rethinking the entire domain modeling. :)

As you mentioned, should a user have a profile not tied to a business or developer? Maybe that's where the avatar lives? We could migrate the relevant data from Developer and Business to the UserProfile (or whatever we call it). Then Developer becomes DeveloperProfile and Account could have a delegated type that includes the developer or business.

Sorry to derail but I think I need to think on this a bit more. This thread plus the Twitter convo with @kaspth really has my head spinning (in a good way!).

jayeclark commented 2 years ago

@joemasilotti Yeah, I’ve been moving slowly on this not so much because of unfamiliarity with Rails, but more that the more I think it through, the more I see other areas of the codebase that may need to change or be thought through first.

Conversations is a big one. Currently (to avoid blowing up the codebase) I’m conceiving of it that a business owner can see all the conversations but would need to actively take over a conversation they didn’t initiate in order to message the developer. (And it would be clear that someone new at Business XYZ was sending the message.) The conversation would still be visible to the original Business user up until the point it was taken over. If a user’s association with a business gets severed, they lose access to all conversations that they initiated on behalf of that business. I’m going to dive in to conversations this weekend and should have a few screenshots to share that can better communicate where my head is on this.

joemasilotti commented 2 years ago

Wanted to give you a heads up that I took (yet) another spin on this Accounts vs. Users thing today. PR #508 has some scaffolding for the schema and data migrations on how to get us started in a slightly different direction.

Curious to hear your thoughts on that and how they align with your PR!

jayeclark commented 2 years ago

Hmm, at first glance it seems to be way overcomplicating things, so there must be something I'm missing. Will take a closer look later!

joemasilotti commented 2 years ago

Ha, fair! I think the PR could lay a groundwork for something more robust than what we currently need. But will make it easier to extend than what we've been talking about.

I'm not sure if I'm overoptimizing or if it is thinking ahead.

jayeclark commented 2 years ago

OK, I took a closer look and it's more closely aligned than I initially thought. Definitely a better framework if it's likely that you'll want to extend account memberships to the developer side of things. And I can see the value of being able to switch account context - it feels slightly like overkill at the moment but will be increasingly useful as new features get added and there is more to 'do' in either context (right now if I'm a developer all I can really do in the app is update my profile and respond to messages, and if I'm a business I can search for/reach out to developers, update my profile, and respond to messages - it almost doesn't feel 'meaty' enough to be swapping between Developer & Business mode, if that makes sense?)

I left a comment in that other PR but the one thing that's still really tripping me up is the lack of a central user identity & profile that I retain as I switch context among these various accounts.

Right now, in the other PR, it feels like it's heading down the path of Twitter/Tweetdeck - ie you give others permission to post & DM on behalf of your twitter account, and when they do, you can't tell that it's not you tweeting... but this can also make it tough to track down bad actors and can be a jarring experience from a customer service perspective since the user may not be sure who they're DMing with at any given time.

An alternative framework would be something like AirBNB, where each person has a user profile that's extended by additional info based on whether they're in 'guest' or 'host' mode, and then within host mode they can be associated with and act on behalf of a number of different properties (as owner/host, co-host, manager, etc. - but in each conversation the guest will see that it is a specific person interacting with them, not just the business.)

Is either of those scenarios what you're going for @joemasilotti ? 😆

joemasilotti commented 2 years ago

Thanks for calling this out! In the long run I'd like railsdevs to function more like Airbnb instead of Tweetdeck. But I'm not quite sure what the immediate next steps should be.

jayeclark commented 2 years ago

Could it be as simple (on the schema side) as making User avatarable and adding name fields to each user record?

(For existing users, name could be extracted from the Developer profile, and if there's no Dev profile then the name could be pulled from the business contact name.)

Sign up flow would probably need to change slightly, as would conversations.

joemasilotti commented 2 years ago

That's a great call. We have all the data we need to backfill.

I love the idea of collecting name (or not) during sunup but having a new profile edit screen for name and avatar. It could even render above the developer/business form as nested attributes!

jayeclark commented 2 years ago

I think it might make sense to land that other PR and then have me change this one to focus on the user profile changes. It makes a lot of sense to me to solidify the concept of a user (that remains the same regardless of the context that user is operating in) & clean up how users are onboarded/represented in conversations before adding visible/perceptible complexity to the entities that any given user can be associated with. (But setting up the accounts framework behind the scenes first would make it a lot easier to migrate conversations.)

I could revert my changes in this PR so far and instead focus on those user data migrations once the accounts framework is set up.

What do you think?

joemasilotti commented 2 years ago

It makes a lot of sense to me to solidify the concept of a user (that remains the same regardless of the context that user is operating in)

I agree. And I like where this conversation has navigated the decision.

I could revert my changes in this PR so far and instead focus on those user data migrations once the accounts framework is set up.

Your call! You are also welcome to contribute to the data migrations PR if you want to work on that.

jayeclark commented 2 years ago

Haven't forgotten about this! I thought about it some more and I don't think the user migration needs to wait for the accounts/data migration PR, and it might even be better if it happens beforehand. It probably makes sense to split the user work into two PRs:

PR 1: Basic user profile data migration (mainly backend + small signup flow UI tweak)

  1. Make user avatarable and add 'name' to the user model
  2. Copy avatars from developer profiles (if they are present). For users with a business avatar and no developer avatar, it seems like that avatar is likely to be a logo, so should not be copied to be their user profile image.
  3. Copy name from either developer (preferred) or business contact name (if there is no developer name.)
  4. Tweak signup process to include name (required) and avatar (optional).

PR 2: UI and additional backend tweaks

  1. Add users to conversations & messages. (Currently the user identification is based on the business & developer connected to a conversation, but in a situation where there will potentially be multiple users per business or developer, the conversation and messages need to specify which specific users have access to the conversation.)
  2. Make similar updates to notifications.
  3. Update conversation UI to reflect user identities as well as business identities.
  4. Update signed in user component to show the user avatar (and display user name on drop down) and ensure users can update their avatar and name
  5. Remove contact name from business profile & developer profile since it now exists on the user profile, update UI for business and developer profiles as needed

I'm going to revert the existing commits on PR #490 & start adding new ones over the weekend on this new tack, but let me know if anything jumps out in the list above as immediately 'off' or overlooked @joemasilotti .

joemasilotti commented 2 years ago

This sounds like a great plan, thank you for outlining it! Just a few questions/concerns:

  1. I don't want to add any new fields to the sign up screen. So we need a way to gather the name/avatar after sign up. Maybe this is a second wizard-like step or maybe it is a nested form in the developer/business flow. The latter is the cleanest but it might create issues if someone tries to be both a developer and a business. Curious to hear your thoughts on that.
  2. "Add users to conversations & messages." and "Make similar updates to notifications." – Can we do these without any Account-related code?
jayeclark commented 2 years ago
  1. Yeah, I was imprecise in my language. I should have written "add name and avatar to onboarding flow", not necessarily the initial sign up form. I'm leaning toward a nested form in both the developer and business flow that only appears if the info has not yet been provided. (So, if I sign up as a business and then later go to create a developer profile, I will not be prompted for my name & avatar, since I already provided those in the business onboarding flow. However, there will be an option in the signed in user component where I can update my name & avatar if I so desire.)

  2. Yes, after thinking about it a bit and poking around in the code, it seems like the most important changes to conversations, messages, and notifications to separate users from the Developer or Business profiles they're acting on behalf of, can be done without any of the account scaffolding in place, and shouldn't be affected by how the account scaffolding eventually shakes out. Some additional changes will need to be made along with the Account-related code (adding a check that a given user has permission to view a conversation they were participating in, ie that particular user has an active relationship with the developer or business account) but these are additive and wouldn't affect the changes I'm proposing, I don't think.

joemasilotti commented 2 years ago
  1. I love it. That creates effectively zero change for a new user. And an existing one will have to change their name/avatar from a new, dedicated form, which is fine by me.
  2. Sounds good!

One more question: if a business goes to message a developer they won't have an avatar until they manually set one. Should we fallback to the business "logo" or force them to add a new avatar?

sarvaiyanidhi commented 1 year ago

Hi @joemasilotti .. As discussed, I would like to work on this issue. Will go through current comments and come up with details to get this working.

joemasilotti commented 1 year ago

Closed in favor of a fresh start in #843.