-
Notifications
You must be signed in to change notification settings - Fork 86
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
User invites #1351
User invites #1351
Conversation
lib/code_corps/model/user_invite.ex
Outdated
|
||
schema "user_invites" do | ||
field :email, :string, null: false | ||
field :role, :string, default: "contributor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't have a default because it's possible for an invite to not be for a project.
create table(:user_invites) do | ||
add :email, :string, null: false | ||
add :name, :string, null: true | ||
add :role, :string, null: true, default: "contributor" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here re: default.
-- Dumped from database version 10.1 | ||
-- Dumped by pg_dump version 10.1 | ||
-- Dumped from database version 10.0 | ||
-- Dumped by pg_dump version 10.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we see if there's a way to avoid the db version numbering while still getting the benefits of the structure.sql?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing any documented way of doing so with mix ecto.dump
%UserInvite{} | ||
|> Changeset.cast(params, [:email, :name, :role, :inviter_id, :project_id]) | ||
|> Changeset.validate_required([:email, :inviter_id]) | ||
|> Changeset.validate_inclusion(:role, ~w(contributor admin owner)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally we would source the role types from the ProjectUser
.
|
||
if count > 0 do | ||
changeset |> Changeset.add_error(:email, "Already associated with a project member") | ||
else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the requirement wasn't quite clear here: we want to be sure that the email is not associated with a User
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Several questions then
-
will a project admin/owner ever be able to invite an existing codecorps user to their project?
- would that not create a
UserInvite
and create something else? Maybe aProjectUser
record with a status of"invited"
?
- would that not create a
-
what happens in the following scenario:
- invite is created for a "[email protected]"
- later, independently, a user registers with that email
- even later, that same user tries to claim the invite
end | ||
|
||
@spec claim_invite(map) :: {:ok, User.t} | ||
def claim_invite(%{} = params) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll need to accommodate invites for projects and general invites to Code Corps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, the code sort of does that. It will
- create a user
- join that user with the project, provided the project and role are specified
- assign that user to the assignee
I'm guessing an email will also be involved, which should be pluggable into this process
|
||
@spec claim_new_user(map) :: {:ok, User.t} | ||
defp claim_new_user(%{} = params) do | ||
%User{} |> User.registration_changeset(params) |> Repo.insert |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this will change, but in all the 1.6 auto-formatting I've seen everything uses ()
, so Repo.insert()
.
49c50ca
to
231f337
Compare
Yes, they would be able to invite an existing user. But this would be simply a
Hadn't thought of this. Ideally upon registration we would look through the |
- Add UserInvite schema and model - Add Accounts.UserInvites subcontext - Add Accounts.create_invite tests
- Add Accounts.claim_invite/1 tests - Add validation step to Accounts.create_invite, to ensure invitee is not a member of the project already
- includes tracking of event
- required refactoring of Accounts.create and Accounts.UserInvites.claim_invite
lib/code_corps/accounts/accounts.ex
Outdated
end | ||
def create(%{} = params) do | ||
%User{} |> User.registration_changeset(params) |> Repo.insert() | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We needed to extract creation into a context. I didn't have a better idea other than this multiple clause approach to differentiate between plain account creation and an invite claim.
""" | ||
@spec roles :: list(String.t()) | ||
def roles do | ||
~w{pending contributor admin owner} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an auto-formatting of the file as I was making the roles
function public (to be used when validating a UserInvite
:stripe_platform_customer, | ||
:user_categories, | ||
:user_roles, | ||
:user_skills | ||
] | ||
|
||
def preload(data) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another auto-formatting of file as I was modifying the create
function. Actual changes are in the function above.
[_role, _project_id] -> | ||
changeset | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping there's a shorter way to write this, but couldn't think of anything.
@joshsmith I think this is finally in a mergeable state. There are issues to be created to expand this further, pending feedback, but other than that, I think our initial bases are covered. I'll update the PR description with the current state of the PR |
@begedin remember to explicitly re-request a review from me so that it will show up in my reviews. |
Additional tracking to implement here:
I actually think the project specific ones could be ignored. For those, we would could have funnels with custom events instead, since the I'm not sure if we might also need a
|
@joshsmith I got a question on tracking based on info from #1364 The rest of the code is good for another review. |
e075407
to
6d6cc63
Compare
What's in this PR?
This PR adds API support for
UserInvite
creation and claims.The Issue this is referring to specifies that we should be adding an
InvitedUser
model, butUserInvite
seems more consistent in naming, so I opted for that.Creation of user invites
Claiming of user invites
This is done through the user controller create action. The user creation was extracted into
Accounts.create/1
and a new, special clause matches against a payload holding an"invite_id"
key, to differentiate from normal account creation.When claiming the invite we
invitee
The user controller is where we track any claimed invites.
Suggested issues to create
References
Closes #1350