Skip to content

Commit

Permalink
Refactor: Remove devise_invitable and simplify invitation workflow
Browse files Browse the repository at this point in the history
- Removed `devise_invitable` gem from the application.
- Simplified the invitation process by eliminating the need to invite both the `User` model and the `Membership` model.
  - Invitations are now handled solely through the `Membership` model.
- This refactor addresses several issues:
  - Reduced complexity in the invitation workflow, making it easier to maintain and understand.
  - Fixed edge cases where duplicate or inconsistent invitations occurred due to the dual-model approach.
  - Streamlined user creation and association, ensuring a smoother onboarding experience.
- Updated relevant tests and documentation to reflect the new workflow.

This change simplifies the codebase and enhances the reliability of the invitation system while maintaining all existing functionality.
  • Loading branch information
koetsier committed Jan 30, 2025
1 parent 5cdb620 commit bd1e069
Show file tree
Hide file tree
Showing 60 changed files with 1,167 additions and 970 deletions.
1 change: 0 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ gem "aws-sdk-route53", "~> 1.107.0"
gem "aws-sdk-s3", "~> 1.144.0"
gem "cancancan"
gem "devise", "~> 4.9.3"
gem "devise_invitable", "~> 2.0.9"
gem "devise_zxcvbn", "~> 6.0.0"
gem "elasticsearch", "~> 8.14.0"
gem "google-apis-drive_v3"
Expand Down
4 changes: 0 additions & 4 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,6 @@ GEM
railties (>= 4.1.0)
responders
warden (~> 1.2.3)
devise_invitable (2.0.9)
actionmailer (>= 5.0)
devise (>= 4.6)
devise_zxcvbn (6.0.0)
devise
zxcvbn (~> 0.1.7)
Expand Down Expand Up @@ -559,7 +556,6 @@ DEPENDENCIES
capybara
debug
devise (~> 4.9.3)
devise_invitable (~> 2.0.9)
devise_zxcvbn (~> 6.0.0)
elasticsearch (~> 8.14.0)
erb_lint
Expand Down
34 changes: 1 addition & 33 deletions app/controllers/memberships_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,7 @@ class MembershipsController < ApplicationController
before_action :validate_preserve_admin_permissions, only: %i[update destroy], unless: :super_admin?
skip_before_action :redirect_user_with_no_organisation, only: %i[destroy edit update]

def edit
@permission_level_data = [
OpenStruct.new(
value: Membership::Permissions::ADMINISTRATOR,
text: "Administrator",
hint: <<~HINT.html_safe,
<span>View locations and IPs, team members, and logs</span><br>
<span>Manage locations and IPs</span><br>
<span>Add or remove team members</span><br>
<span>View, add and remove certificates</span>
HINT
),
OpenStruct.new(
value: Membership::Permissions::MANAGE_LOCATIONS,
text: "Manage Locations",
hint: <<~HINT.html_safe,
<span>View locations and IPs, team members, and logs</span><br>
<span>Manage locations and IPs</span><br>
<span>Cannot add or remove team members</span><br>
<span>View, add and remove certificates</span>
HINT
),
OpenStruct.new(
value: Membership::Permissions::VIEW_ONLY,
text: "View only",
hint: <<~HINT.html_safe,
<span>View locations and IPs, team members, and logs</span><br>
<span>Cannot manage locations and IPs</span><br>
<span>Cannot add or remove team members</span>
HINT
),
]
end
def edit; end

def update
permission_level = params.permit(:permission_level).fetch(:permission_level)
Expand Down
31 changes: 14 additions & 17 deletions app/controllers/users/confirmations_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,33 @@ class Users::ConfirmationsController < Devise::ConfirmationsController
# This controller overrides the Devise:ConfirmationsController (part of Devise gem)
# in order to set user passwords after they have confirmed their email. This is
# based largely on recommendations here: 'https://github.com/plataformatec/devise/wiki/How-To:-Override-confirmations-so-users-can-pick-their-own-passwords-as-part-of-confirmation-activation'
before_action :set_user, only: %i[update show]
before_action :ensure_user_not_confirmed, only: %i[update show]
before_action :fetch_organisations_from_register, only: %i[update show]
after_action :publish_organisation_names, only: :update

rescue_from UserMembershipForm::InvalidTokenError, with: :error_handler
rescue_from UserMembershipForm::AlreadyConfirmedError, with: :error_handler

# resend confirmation
def new; end

# resend confirmation
def create
User.send_confirmation_instructions(email: params[:email])
set_flash_message! :notice, :send_paranoid_instructions
redirect_to after_resending_confirmation_instructions_path_for(User)
end

# confirm user
def show
params = token_params.empty? ? form_params : token_params
@form = UserMembershipForm.new(params)
@form = UserMembershipForm.new(token_params.empty? ? form_params : token_params)
end

# confirm user
def update
@form = UserMembershipForm.new(form_params)
if @form.write_to(@user)
if @form.save
set_flash_message :notice, :confirmed
sign_in_and_redirect(resource_name, @user)
sign_in_and_redirect(resource_name, @form.user)
else
render :show
end
Expand All @@ -34,17 +38,6 @@ def pending; end

protected

def set_user
@user = User.find_or_initialize_with_error_by(:confirmation_token, token_params[:confirmation_token] || form_params[:confirmation_token])
end

def ensure_user_not_confirmed
if @user.confirmed?
flash[:alert] = "Email was already confirmed"
render "users/confirmations/new"
end
end

def token_params
params.permit(:confirmation_token)
end
Expand All @@ -55,6 +48,10 @@ def form_params

private

def error_handler(exception)
redirect_to root_path, alert: exception.message
end

def fetch_organisations_from_register
@register_organisations = Organisation.fetch_organisations_from_register
end
Expand Down
138 changes: 50 additions & 88 deletions app/controllers/users/invitations_controller.rb
Original file line number Diff line number Diff line change
@@ -1,117 +1,79 @@
class Users::InvitationsController < Devise::InvitationsController
before_action :show_navigation_bars
before_action :delete_user_record, if: :user_should_be_cleared?, only: :create
after_action :confirm_new_user_membership, only: :update # rubocop:disable Rails/LexicallyScopedActionFilter
class Users::InvitationsController < ApplicationController
skip_before_action :authenticate_user!, :redirect_user_with_no_organisation, only: %i[edit update]
before_action :validate_permissions, only: %i[new create]
rescue_from AcceptInvitationForm::InvalidTokenError, with: :invalid_token_handler
# new invitation
def new
@invitation_form = InvitationForm.new
end

# send the invitation
def create
if user_is_invalid? || user_belongs_to_current_organisation?
self.resource = invite_resource
render(params[:user][:source] == "invite_admin" ? :invite_second_admin : :new)
return
end

self.resource = invite_resource unless user_belongs_to_other_organisations?
@invitation_form = InvitationForm.new(invitation_params)
if @invitation_form.valid?
@invitation_form.save!

add_user_to_organisation(organisation)

redirect_to(memberships_path, notice: "#{invited_user.email} has been invited to join #{organisation.name}")
redirect_to memberships_path, notice: "#{@invitation_form.email} has been invited to join #{current_organisation.name}"
else
render(params.dig(:invitation_form, :source) == "invite_admin" ? :invite_second_admin : :new)
end
end

def invite_second_admin
@user = User.new
# invitee accepts the invitation - ask name / password if required
def edit
@form = AcceptInvitationForm.new(invitation_token: params[:invitation_token])
end

private

def show_navigation_bars
false if action_name == "invite_second_action"
end

def organisation
current_organisation
# invitee accepts the invitation
def update
invitation_params = params.require(:accept_invitation_form).permit(:password, :name, :invitation_token)
@form = AcceptInvitationForm.new(invitation_params)
if @form.save
redirect_to new_user_session_path, notice: "You have successfully joined #{@form.organisation_name}. Please log in."
else
render :edit
end
end

def add_user_to_organisation(organisation)
membership = invited_user.memberships.find_or_create_by!(invited_by_id: current_user.id, organisation:)
membership.permission_level = params[:permission_level]
membership.save!
send_invite_email(membership) if user_has_confirmed_account?
end
def resend
email = params[:email]
user = User.find_by_email(email)
membership = user&.membership_for(current_organisation)
raise "Cannot find membership for user #{email} and organisation #{current_organisation.name}" if user.nil? || membership.nil?
raise "The membership for user #{email} and organisation #{current_organisation.name} has already been confirmed" if membership.confirmed?

def send_invite_email(membership)
GovWifiMailer.membership_instructions(
invited_user,
user,
membership.invitation_token,
organisation: membership.organisation,
).deliver_now
redirect_to memberships_path
end

def user_has_confirmed_account?
invited_user.confirmed?
end

def user_belongs_to_current_organisation?
invited_user&.confirmed? && invited_user.organisations.include?(organisation)
end

def user_belongs_to_other_organisations?
invited_user.present? &&
invited_user.confirmed? &&
invited_user.organisations.pluck(:id).exclude?(current_organisation&.id)
end

def authenticate_inviter!
# https://github.com/scambra/devise_invitable#controller-filter
unless current_user&.can_manage_team?(current_organisation)
redirect_to(root_path) and return
end
end

def delete_user_record
invited_user.destroy! unless invited_user.nil?
end

def user_is_invalid?
!invite_params[:email].match? Devise.email_regexp
end

def invited_user
User.find_by(email: invite_params[:email])
end

def resending_invite?
!!params[:resend]
end

def user_should_be_cleared?
resending_invite? || unconfirmed_user_with_no_org?
def invite_second_admin
@form = InvitationForm.new
end

def unconfirmed_user_with_no_org?
invited_user_already_exists? &&
invited_user_not_confirmed? &&
invited_user_has_no_org? &&
!invited_user.is_super_admin?
end
private

def invited_user_already_exists?
!!invited_user
def invalid_token_handler(exception)
redirect_to root_path, alert: exception.message
end

def invited_user_not_confirmed?
!invited_user.confirmed?
def validate_permissions
redirect_to(root_path) unless current_user.can_manage_team?(current_organisation)
end

def invited_user_has_no_org?
invited_user.organisations.empty?
def invitation_token
params[:invitation_token] || params.dig(:accept_invitation_form, :invitation_token)
end

def confirm_new_user_membership
current_user.default_membership.confirm! if current_user
def invitation_params
parameters = params.require(:invitation_form).permit(:email, :permission_level)
parameters.merge(inviter: current_user, organisation: current_organisation)
end

# Overrides https://github.com/scambra/devise_invitable/blob/master/app/controllers/devise/invitations_controller.rb#L105
def invite_params
params.require(:user).permit(:email)
def show_navigation_bars
action_name != "invite_second_action"
end
end
37 changes: 37 additions & 0 deletions app/helpers/permissions_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# frozen_string_literal: true

module PermissionsHelper
def permissions_radio_data
[
OpenStruct.new(
value: Membership::Permissions::ADMINISTRATOR,
text: "Administrator",
hint: <<~HINT.html_safe,
<span>View locations and IPs, team members, and logs</span><br>
<span>Manage locations and IPs</span><br>
<span>Add or remove team members</span><br>
<span>View, add and remove certificates</span>
HINT
),
OpenStruct.new(
value: Membership::Permissions::MANAGE_LOCATIONS,
text: "Manage Locations",
hint: <<~HINT.html_safe,
<span>View locations and IPs, team members, and logs</span><br>
<span>Manage locations and IPs</span><br>
<span>Cannot add or remove team members</span><br>
<span>View, add and remove certificates</span>
HINT
),
OpenStruct.new(
value: Membership::Permissions::VIEW_ONLY,
text: "View only",
hint: <<~HINT.html_safe,
<span>View locations and IPs, team members, and logs</span><br>
<span>Cannot manage locations and IPs</span><br>
<span>Cannot add or remove team members</span>
HINT
),
]
end
end
8 changes: 4 additions & 4 deletions app/mailers/gov_wifi_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,17 +37,17 @@ def unlock_instructions(record, token, _opts = {})
def invitation_instructions(record, token, _opts = {})
opts = {
email_address: record.email,
personalisation: { invite_url: accept_invitation_url(record, invitation_token: token) },
personalisation: { invite_url: accept_user_invitation_url(record, invitation_token: token) },
template_id: NotifyTemplates.template(:invite_email),
reference: "invite_email",
}
Services.notify_gateway.send_email(opts)
end

def membership_instructions(record, token, opts = {})
def membership_instructions(record, invitation_token, opts = {})
opts = {
email_address: record.email,
personalisation: { invite_url: confirm_new_membership_url(token:),
personalisation: { invite_url: edit_users_invitation_url(invitation_token:),
organisation: opts.fetch(:organisation).name },
template_id: NotifyTemplates.template(:cross_organisation_invitation),
reference: "invite_email",
Expand All @@ -58,7 +58,7 @@ def membership_instructions(record, token, opts = {})
def nomination_instructions(name, email_address, nominated_by, organisation, token)
opts = {
email_address:,
personalisation: { nomination_url: new_nominated_mou_path(token:), name:, nominated_by:, organisation: },
personalisation: { nomination_url: new_nominated_mou_url(token:), name:, nominated_by:, organisation: },
template_id: NotifyTemplates.template(:nominate_user_to_sign_mou),
reference: "nomination_email",
}
Expand Down
Loading

0 comments on commit bd1e069

Please sign in to comment.