Bulk invite team members (setup section II) #3143
Conversation
paolodamico
force-pushed
the
setup-2-invite-members
branch
from
a34fb2e to
ea2677f
Compare
|
FYI @Twixes too ready for review. |
| ) | ||
| target_email: models.EmailField = models.EmailField(null=True, db_index=True) | ||
| first_name: models.CharField = models.CharField(max_length=30, blank=True, default="") |
There was a problem hiding this comment.
Field name is first_name, but is this actually intended for first name explicitly? If this is only a holdover from User.first_name (which is kind of a legacy issue), then it should probably be just name
There was a problem hiding this comment.
Well it is indeed to match User.first_name, but I thought we were just using first name, not full name everywhere else too (e.g. the signup form says first name and the placeholder says "Jane"). So first_name sounds appropriate even then, thoughts?
There was a problem hiding this comment.
Hmm, I'm pretty sure we're using full names, just using a single field, as we don't use User.last_name anywhere. 🤔
There was a problem hiding this comment.
Hm, actually we are kind of using it… accidentally, as social auth fills it in on registration. We should get this straight either way. Thoughts @paolodamico?
There was a problem hiding this comment.
Well I'm not sure what most users did before we revamped the signup page, but for recent users not doing social auth, I think it's pretty straightforward to just type your first name. I would just do first name everywhere, it has easier backwards support, it's nicer to receive emails addressed like "Hey John! instead of "Hey John Doe!" and I don't think we really need the last name for anything.
The only reason I can think for this is for very large teams, but in that case maybe the email address is enough, or they could just do full name by their own decision?
|
The |
|
Thanks for the review @Twixes! As this introduces quite a few changes to the onboarding process I'll wait until tomorrow to merge to see if @macobo has more input. The not passing test seems like a flaky issue, tests were running before and doesn't look like any relevant front-end code was altered. Running again 👍 As a bit of feedback on the review if its helpful, I think stuff like ae7fc1e just causes us to spend more time on re-reviews and doesn't really provide a lot of value (purely stylistic). |
|
Oh, I just took a look because of the organization-related fix, but @macobo will definitely have valuable input, especially given previous reviews around this project. |
|
Definitely, super helpful you reviewed that stuff too, thanks! Would've loved to do this in two separate PRs but it ended up being too intertwined to be worth it. |
paolodamico
force-pushed
the
setup-2-invite-members
branch
from
e0772be to
e097813
Compare
frontend/src/scenes/organization/TeamMembers/BulkInviteModal.tsx
Outdated
Show resolved
Hide resolved
frontend/src/scenes/organization/TeamMembers/bulkInviteLogic.ts
Outdated
Show resolved
Hide resolved
frontend/src/scenes/organization/TeamMembers/bulkInviteLogic.ts
Outdated
Show resolved
Hide resolved
frontend/src/scenes/organization/TeamMembers/bulkInviteLogic.ts
Outdated
Show resolved
Hide resolved
Changes
Builds up on #3129 to introduce the last step of setup's Section II.
OrganizationInviteto use theUserSerializerinstead of fetching every property individually. Plus, some cleanup to the serializer.Tagging @jamesefhawkins, for feedback on inviting multiple team members on onboarding experience.
Checklist