-
-
Notifications
You must be signed in to change notification settings - Fork 264
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 #835
Conversation
36b5fa4
to
a61c20d
Compare
a61c20d
to
0ae8bed
Compare
All of the core tests are working! This one is ready for review now. |
@gazayas this looks like a super handy feature, but I wonder if we should make a config setting that controls whether it's active or not. It's a pretty big new change and I'm not sure we want to force it on people who have an existing app running. If they have custom tests that cover sign-up & any custom onboarding flows that they've built then this change would break all of those tests. Maybe we could add a toggle to |
@jagthedrummer Done! |
bin/configure
Outdated
end | ||
line | ||
end | ||
File.write("config/initializers/bullet_train.rb", bt_config_lines.join) |
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.
Nice!
@gazayas, that looks great! There's just one test failing both on the |
@jagthedrummer Hmm, I'm not quite sure what's going on with the OpenAPI test, but that's the only one that's failing when HIDE_THINGS is true. It looks like we have
I'm also not entirely sure what removing TangibleThings has to do with this 🤔 Any thoughts? I got the other tests to pass, as well as the tests in the joint PR by the way. |
I'll work on the merge conflict here, I also saw that #912 was opened though so I might wait until we resolve that because the only test that's failing here is the OpenAPI test for Core: Minitest with HIDE_THINGS. |
Yeah, since the OpenAPI stuff is revolving around Invitations API and the subject of this PR is invitations, I'll go ahead and wait a bit. |
@jagthedrummer Besides the release tests (as expected), the tests are passing! |
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.
@gazayas I just pulled this branch down and gave it a run. I have a few more suggestions that I'm going to put here, just so its' all in one place. I realize some of the actual changes may need to happen in the joint PR in core
.
-
I think it should be possible to bulk-invite fewer than three people on this screen. What if there are only one or two other people on my team?
Here I try to submit a single invitation:
And it complains about all three being empty, and it doesn't retain the one email address and role that I did submit.
In the console I see three of this error:
Unpermitted parameter: :role_ids.
- As I noted in an inline comment, the test for not skipping this screen isn't being run in CI because the feature defaults to being turned off. I think we can manually set that config in the test setup, and then restore it during teardown.
test/system/invitation_lists_test.rb
Outdated
require "application_system_test_case" | ||
|
||
class InvitationListsTest < ApplicationSystemTestCase | ||
if bulk_invitations_enabled? |
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 test isn't running because we default to this feature being turned off. I think this test should change the config setting prior to test setup to ensure that this test always run. (And it should restore the original setting afterwards.)
@jagthedrummer Thanks for the review! Here are some thoughts.
Done!
I've been instructed to do three for now, and was told another developer on the team will work on a stimulus controller so we can adjust the amount of people we can invite for these invitations. If anything, I can just set it to one and make a note about the other developer adjusting the JS. Also, concerning Also, will look into making the page with errors retain the values, not quite sure what the cause is right now.
I went ahead and edited the tests so that they run regardless of the setting in |
@gazayas, thanks for the info about the JS portion of this. Are we planning to wait and have that be a part of this PR? Or are we wanting to merge this and then follow up later with the JS stuff? And, if we're planning to wait, is there some way to make the controller just be forgiving about one or more of those email addresses being empty? I think making roles a multi select makes sense given that you can assign multiple roles in other places within the app. (Related, it might be nice to have a BT config option that toggles the |
@jagthedrummer I made Roles multiple, and the placeholder now has With that being said, I'm having trouble getting the emails to reappear in the fields after the validations fail. Right now, for the invitation form for example, I'm passing a new <%= form.fields_for :invitations, Invitation.new do |invitation_form| %>
...
<% end %> I'm seeing though in other invocations we have of I'm assuming this is the issue since the controller has With that being said, I changed the invitation list to only send one invitation because I believe we will implement the JS in another PR. Last thing: I also changed the errors so it shows one general error like |
@gazayas, I just pulled down this branch and gave it another run and I think things are looking good for the portion that we're trying to implement in this PR. Since we're not really considering this to be a completed feature (due to the JS portion for "add more team members") I think we should probably disable this bit from Other than than I think we can merge this once merge conflicts are resolved and tests are passing in both PRs. |
@jagthedrummer Done! |
* Prepare database for invitation lists * Skip bulk invitation step in most system tests * Add tests for bulk invitations * Fixing Standard Ruby * Add another assertion for InvitationList * Extract invitation lists test logic to its own file * Add another skip in invitations test * Add comment in BT initializer for bulk invitations * Only test bulk invitations if enabled * Enable bulk invitations on bin/configure step * Fixing Standard Ruby * Edit tests to always run invitation lists system tests * Update schema * Remove non-existent error assertion * Show correct error message in invitation lists test * Try to fix invitation lists test * Remove syntax error * Fixing Standard Ruby
Joint PR
Note:
This PR is shipping a new feature that will be disabled by default. The feature is only partially complete, but we want to get this merged so that other team members can work on additional parts of the feature.
If you'd like to opt-in to this work-in-progress feature, you can add a line to
config/initializers/bullet_train.rb
like this: