Allow admins to invite other staff #251

marlena-b · 2023-10-10T14:32:28Z

🔗 Issue

Fixes #226

✍️ Description

This Pull Request adds the functionality of inviting new staff members to the organization by administrators (staff admins) using the devise_invitable gem. The invitation acceptance flow is currently in a basic form but improvements will be addressed in a subsequent PR.

📷 Screenshots/Demos

Nagranie.z.ekranu.2023-10-11.o.13.25.13.mov

kasugaijin · 2023-10-12T22:42:57Z

app/controllers/organizations/invitations_controller.rb

+    if @user.save
+      @user.staff_account.add_role(user_params[:staff_account_attributes][:roles])
+      @user.invite!(current_user)
+      redirect_to staff_index_path, notice: "Staff saved successfully."


Can we change the notice to something like "Invite sent!"?

I think both make sense here. The staff account does get saved and the invitation is sent

I think "Invite sent!" makes more sense from the user perspective. Fixed!

I also changed texts in a couple of other places from "Add staff" to "Invite staff".

kasugaijin · 2023-10-12T22:53:57Z

app/views/organizations/invitations/_form.html.erb

+
+    <!-- button -->
+    <div class="col-12 mt-3">
+      <%= form.submit 'Save profile', class: 'btn btn-primary' %>


Can we change button to 'Send Invite' ?

Fixed, thanks!

kasugaijin

Looks great! I tested it out locally and I like it.

I know you said you had improvements to follow in another PR, so the following is my thoughts, some which you may already have considered:

I am guessing the invite! call on line 16 of Organizations::InvitationsController is triggering the mailer. I am wondering if we are able to send any params to that? It would be good to send the organization so we can access its name and slug and recreate the Org's Url in the email e.g., https://www.petrescue.com/baja
The redirect after the invited user enters and saves their password to accept the invite, is this coming from registrations_controller def after_sign_in_path_for ? This is another place where it would be good to scope it to the organization's dashboard rather than the root path, if possible.
I think we could use the invitation_accepted_at column in the staff table to indicate whether or not the staff has accepted the invite in the staff table (not directly related to this work) e.g., in the status column

kasugaijin · 2023-10-12T23:28:21Z

db/migrate/20231009132343_devise_invitable_add_to_users.rb

@@ -0,0 +1,26 @@
+class DeviseInvitableAddToUsers < ActiveRecord::Migration[7.0]


My initial thought without considering the gem would be to separate Invitations from Users and have an Invitations table/model. This would be more maintainable in the long run.

However, looking at the gem, I see that adding to Users table is a 'normal' practice. I was thinking it doesn't make sense if only Org users use invites, as then we'd have a User class that for adopters has completely irrelevant fields. But, then I thought we could also leverage invitations between adopters as well at some point.

This is not to say this needs to change. I think it works like this and is ok a. But, I think with more time and a bigger budget I'd go for separating those concerns :D

I totally agree with your point and would love to make those changes too, but sadly, this gem doesn't let us do that (there is even an issue for this: scambra/devise_invitable#888).

Let's go with what we have for now and think about extracting it later on if needed.

nsiwnf · 2023-10-13T03:05:23Z

config/locales/devise_invitable.en.yml

+        someone_invited_you: "Someone has invited you to %{url}, you can accept it through the link below."
+        accept: "Accept invitation"
+        accept_until: "This invitation will be due in %{due_date}."
+        ignore: "If you don't want to accept the invitation, please ignore this email. Your account won't be created until you access the link above and set your password."


It seems like the account is still created? Or does devise_invitation do something to the User record that gets created?

Hey @nsiwnf, this text was generated by Devise, and TBH I don't know why it says so. The user record will be created in the DB but with a random password so no one can access it. I will remove the Your account won't be created... sentence.

nsiwnf · 2023-10-13T03:07:23Z

These are great suggestions! Should we make them into tickets?

Looks great! I tested it out locally and I like it.

I know you said you had improvements to follow in another PR, so the following is my thoughts, some which you may already have considered:

* I am guessing the `invite!` call on line 16 of `Organizations::InvitationsController` is triggering the mailer. I am wondering if we are able to send any params to that? It would be good to send the organization so we can access its name and slug and recreate the Org's Url in the email e.g., https://www.petrescue.com/baja

* The redirect after the invited user enters and saves their password to accept the invite, is this coming from registrations_controller   def after_sign_in_path_for ? This is another place where it would be good to scope it to the organization's dashboard rather than the root path, if possible.

* I think we could use the invitation_accepted_at column in the staff table to indicate whether or not the staff has accepted the invite in the staff table (not directly related to this work) e.g., in the status column

nsiwnf

LGTM!

marlena-b · 2023-10-13T11:37:35Z

Thank you @kasugaijin and @nsiwnf for the review! 💪

I will create separate issues for your suggestions and start working on them soon :)

nsiwnf linked an issue Oct 11, 2023 that may be closed by this pull request

Send new staff an invitation to the org #226

Closed

marlena-b added 2 commits October 11, 2023 15:06

Allow admins to invite other staff

51706ee

Add invite staff test

d9c79a1

marlena-b force-pushed the devise-invitable branch from ff9805e to d9c79a1 Compare October 11, 2023 13:07

marlena-b marked this pull request as ready for review October 11, 2023 13:26

marlena-b requested review from nsiwnf, edwinthinks and Thrillberg October 11, 2023 13:26

kasugaijin reviewed Oct 12, 2023

View reviewed changes

kasugaijin approved these changes Oct 12, 2023

View reviewed changes

kasugaijin reviewed Oct 12, 2023

View reviewed changes

nsiwnf reviewed Oct 13, 2023

View reviewed changes

nsiwnf approved these changes Oct 13, 2023

View reviewed changes

Implement CR remarks

ee4b667

marlena-b force-pushed the devise-invitable branch from fa3ed42 to ee4b667 Compare October 13, 2023 11:34

marlena-b merged commit e514793 into main Oct 13, 2023
3 checks passed

marlena-b deleted the devise-invitable branch October 13, 2023 11:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow admins to invite other staff #251

Allow admins to invite other staff #251

marlena-b commented Oct 10, 2023 •

edited

kasugaijin Oct 12, 2023 •

edited

nsiwnf Oct 13, 2023

marlena-b Oct 13, 2023

kasugaijin Oct 12, 2023

marlena-b Oct 13, 2023

kasugaijin left a comment •

edited

kasugaijin Oct 12, 2023

marlena-b Oct 13, 2023

nsiwnf Oct 13, 2023

marlena-b Oct 13, 2023 •

edited

nsiwnf commented Oct 13, 2023

nsiwnf left a comment

marlena-b commented Oct 13, 2023

		@@ -0,0 +1,26 @@
		class DeviseInvitableAddToUsers < ActiveRecord::Migration[7.0]

Allow admins to invite other staff #251

Allow admins to invite other staff #251

Conversation

marlena-b commented Oct 10, 2023 • edited

🔗 Issue

✍️ Description

📷 Screenshots/Demos

kasugaijin Oct 12, 2023 • edited

Choose a reason for hiding this comment

nsiwnf Oct 13, 2023

Choose a reason for hiding this comment

marlena-b Oct 13, 2023

Choose a reason for hiding this comment

kasugaijin Oct 12, 2023

Choose a reason for hiding this comment

marlena-b Oct 13, 2023

Choose a reason for hiding this comment

kasugaijin left a comment • edited

Choose a reason for hiding this comment

kasugaijin Oct 12, 2023

Choose a reason for hiding this comment

marlena-b Oct 13, 2023

Choose a reason for hiding this comment

nsiwnf Oct 13, 2023

Choose a reason for hiding this comment

marlena-b Oct 13, 2023 • edited

Choose a reason for hiding this comment

nsiwnf commented Oct 13, 2023

nsiwnf left a comment

Choose a reason for hiding this comment

marlena-b commented Oct 13, 2023

marlena-b commented Oct 10, 2023 •

edited

kasugaijin Oct 12, 2023 •

edited

kasugaijin left a comment •

edited

marlena-b Oct 13, 2023 •

edited