Skip to content
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

[Pre-release] Enable bulk invitations on onboarding step #394

Merged
merged 30 commits into from
Sep 19, 2023

Conversation

gazayas
Copy link
Contributor

@gazayas gazayas commented Aug 10, 2023

Joint PR:

See the joint PR bullet-train-co/bullet_train#835 for additional information.

Original PR description:

Currently looks like this, I definitely have a lot more work to do on it though.

スクリーンショット 2023-08-10 21 28 53

@gazayas
Copy link
Contributor Author

gazayas commented Aug 10, 2023

I'm thinking we can add a skip button as well. That might help so we don't have to edit our system tests too heavily.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 15, 2023

I'm currently stuck on this one, specifically with how to apply strong parameters. We have to accept nested attributes for each Invitation AND a Membership. I'm sure it's something small I'm missing, just having some trouble with this one.

Unpermitted parameters: :authenticity_token, :account_onboarding_invitation_list, :commit. Context: { controller: Account::Onboarding::InvitationListsController, action: create, request: #<ActionDispatch::Request:0x000000011b1738f8>, params: {"authenticity_token"=>"[FILTERED]", "account_onboarding_invitation_list"=>{"team_id"=>"1", "creator_membership_id"=>"1", "invitations_attributes"=>{"0"=>{"email"=>"[email protected]", "membership_attributes"=>{"role_ids"=>"Foo"}}, "1"=>{"email"=>"[email protected]", "membership_attributes"=>{"role_ids"=>"Foo"}}, "2"=>{"email"=>"[email protected]", "membership_attributes"=>{"role_ids"=>"Foo"}}}}, "commit"=>"Next", "controller"=>"account/onboarding/invitation_lists", "action"=>"create"} }
Completed 400 Bad Request in 10ms (ActiveRecord: 2.0ms | Allocations: 4298)


  
ActionController::ParameterMissing (param is missing or the value is empty: {:invitations_attributes=>[:email, {:membership_attributes=>[{:role_ids=>[]}]}]}):
  
actionpack (7.0.7) lib/action_controller/metal/strong_parameters.rb:477:in `require'
local/bullet_train-core/bullet_train/app/controllers/concerns/account/onboarding/invitation_lists/controller_base.rb:29:in `invitation_list_params'
cancancan (3.5.0) lib/cancan/controller_resource_sanitizer.rb:10:in `sanitize_parameters'

@gazayas
Copy link
Contributor Author

gazayas commented Aug 15, 2023

Currently now just getting this error
スクリーンショット 2023-08-15 17 15 06

@gazayas
Copy link
Contributor Author

gazayas commented Aug 15, 2023

Skip button added

@gazayas
Copy link
Contributor Author

gazayas commented Aug 15, 2023

Taking a step back from it, we probably don't even need the new model. Will try to do something similar to Invitation#resend, although I'll have to play by ear how validations will work out.

@olbrich
Copy link
Contributor

olbrich commented Aug 15, 2023

I expect that users will pretty quickly want to be able to upload a CSV/XLSX of names and emails to invite, particularly if you are dealing with larger teams.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 15, 2023

@olbrich Seems like something we could potentially do with Action Models, will try to keep that in the back of my mind.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 15, 2023

Finding that it's hard to do this one without the existence of a model since Bullet Train fields rely on the existence of a form:

<%= render 'shared/fields/email_field', form: invitation_form, method: :email, options: {autofocus: true} %>

Still running into the strong_params issue, so I think I'll take a step back from this one for a bit to clear my head.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 18, 2023

Made a little more progress on this one. Now we can see if the Invitations and Memberships are valid or not, and it will redirect with an error saying "Please correct the errors below" if something's wrong.

Will have to actually send the invitations next and work on the system tests.

@gazayas
Copy link
Contributor Author

gazayas commented Aug 18, 2023

I have a bunch of hidden fields for now, but I plan on just moving this logic to the controller.

@gazayas gazayas marked this pull request as ready for review August 18, 2023 23:29
@gazayas
Copy link
Contributor Author

gazayas commented Aug 19, 2023

@andrewculver @jagthedrummer The tests are now passing!

This one is ready for review.

invitation.membership.team = current_team

# Role IDs don't get registered automatically because roles_ids is an array, so we handle that here.
if available_roles.include?(params[:account_onboarding_invitation_list][:invitations_attributes][idx.to_s][:membership_attributes][:role_ids].downcase)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably make the params call here shorter, will try to keep this one in the back of my mind.

layout "devise"

before_action do
@user = current_user
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking we might need to do something else for this

@jagthedrummer jagthedrummer changed the title Enable bulk invitations on onboarding step [Pre-release] Enable bulk invitations on onboarding step Sep 19, 2023
@jagthedrummer jagthedrummer merged commit cefa5f7 into main Sep 19, 2023
4 checks passed
@jagthedrummer jagthedrummer deleted the features/onboarding-invitations branch September 19, 2023 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants