Admin, Enterprise Settings, Users tabs: remove tooltips directives + use reflex to invite user to be managers

[binarygit]

What? Why?

Part of #9491

This basically add a reflex for adding a user as manager of an enterprise.
To be more precise:

• Use reflex instead of controller.
• Adds specs for this functionnality
• Remove #!# from URLs as they aren't valid. Commit cherry picked from Admin: Remove #!# anchor from enterprise preferences page #10541. Could be merged before.
• + solves a flaky one (?), not related to this functionality.
• + remplace some tooltips directives

What should we test?

• As an admin, visit /admin/enterprises/ENTERPRISE/edit#/users_panel
• Check that you can still invite some manager to your enterprise. This should be iso-functional as before ;)

Release notes

Changelog Category: Technical changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[dacook]

I found the commit history wasn't straightforward, but they are small enough to understand. I understand this hasn't been easy to pick up :)

I have a few suggestions, can you please take a look?

[dacook]

I remember discussing this with Maikel a while back, and agreeing that we shouldn't have a "catch-all" (rescue StandardError), and should rescue only expected errors. Any unexpected errors should be treated as bugs (logged in bugsnag) so we can improve them.

I can see that the old controller had a problem: create_new_manager could raise an exception (from new_user.save!), but it wasn't being caught.

Handling exceptions in this way is generally a good idea, but for controllers we more commonly use the if/else pattern. So I'd recommend we use that. What do others think?

Otherwise, it should be renamed create_new_manager! and we should only catch expected errors like ActiveRecord::RecordInvalid.

[dacook]

My suggestion (untested):

Suggested change

	begin
	new_user = create_new_manager(email, enterprise)
	locals[:success] = true
	locals[:email] = new_user.email
	rescue StandardError => e
	@error = e.message
	locals[:error] = @error || I18n.t('admin.enterprises.invite_manager.error')
	end
	new_user = create_new_manager(email, enterprise)
	if new_user
	locals[:success] = true
	else
	locals[:error] = new_user.errors.full_messages.to_sentence || I18n.t('admin.enterprises.invite_manager.error')
	end

• This requires that create_new_manager is changed from save! to save
• From what I could see new_user.email should be the same as local var email, so I simplified that too.
• I don't see the need for class var @error, so I removed it

update: I noticed in this PR the use of to_sentence so have incorporated that too.

[jibees]

This requires that create_new_manager is changed from save! to save

Not only, since EnterpriseMailer.manager_invitation(@enterprise, new_user).deliver_later seems to throw exception as well. That's why I've added:

return new_user unless new_user.valid? # Return early if user is invalid.

after new_user.save

[jibees]

Commit: b17d3e4

[dacook]

Great work, thanks for picking that up and making it work.

[jibees]

I have a few suggestions, can you please take a look?

Thanks for those meaningful suggestions! 👍 🙏

I've done, I think, the job, feel free to review. As said on delivery circle meeting, it's not about removing all angular on that panel but only removing some tooltip directives + add manager invitation as reflex.

[dacook]

👍 Agreed, it will be good to have the work from this PR merged.

[mkllnk]

Great work. I would just like to see one little cleanup to make the code more robust.

[jibees]

I would just like to see one little cleanup to make the code more robust.

I agree. Thanks for that!

[filipefurtad0]

Thanks for this!
👏 👏 👏

EDIT: maybe we could update the spec to enqueue two emails jobs?

[jibees]

maybe we could update the spec to enqueue two emails jobs?

expect do
  ...
end.to enqueue_job(ActionMailer::MailDeliveryJob).exactly(:twice)

I did not understand your point ;)

[filipefurtad0]

Ahh right - I've missed it, we do enqueue it twice. All good! 👍
They are enqueued but, not delivered. And the spec does cover for this...

[filipefurtad0]

Hey @binarygit and @jibees ,

Before this PR, the invitation form looks like this:

Sending an invitation delivers two emails - see Wiki on Emails:

• one for account confirmation
• a second email for accepting the management of the enterprise

After this PR

The form has a fresh look:

Sending an invitation triggers this error:

NoMethodError: undefined method `owner' for nil:NilClass
Location
app/mailers/enterprise_mailer.rb:25 - manager_invitation

From bugsnag:

@instance = Spree::Config[:site_name]

 @instance_email = from_address
  
 I18n.with_locale valid_locale(@enterprise.owner) do
 subject = t('enterprise_mailer.invite_manager.subject', enterprise: @enterprise.name)
 mail(to: user.email,
 from: from_address,

And indeed, only one email arrives - the account confirmation. So, the missing email is actual invitation email (email i.)

I think this will need another look, for now as it introduces a regression. Moving to in dev.

[jibees]

Oh yes, there were a copy/paste error (@enterprise instead of enterprise) that couldn't be spotted by specs... ;( Back in Test Ready, since this is pretty small.

[filipefurtad0]

Allright!

Both emails arrive now, and the feature works as before:

Above we see, an accepted invitation, and a pending invitation.

Also, the tooltips on this page - work as before 👍

Looks great to me. Merging!