FEATURE: Various improvements to invite system (PR #12023)



This pull request introduces 1 alert when merging 5896af57d572e01470fa18a582238ac8bcae0e09 into 5352b97f830dd84d97553ae708660cbdc3379068 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

This pull request introduces 1 alert when merging 771c604dd2d63b234a47f72cb89e2dedecb18df5 into 2aac657da7906804e4799bb2e1e35f982391dc87 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

This is confusing to me. Can you explain why we’re making an inviteKey in the client side?

I made some minor comments. Since invites are so important from a security perspective I would like someone else to review the ruby side more closely just to make sure everything looks good.

Controllers shouldn’t be touching the DOM. Can you move this to a component?

Do we have to use style? Using scss is preferred.

I recommend you move the icons out of the translation key and use a font awesome helper.

          no_invite_link: "The invite link will show up here after it's created"

I am generating the invite key on the client side to make it possible to display an invite link even before it is created.

I do not see how we could move this into a component. This is more of a hack to trick the browser into copying some arbitrary text (in this case, invite URL). We do this trick in other places too.

Sorry, I missed this.

Since the invite links are generated when the modal is displayed, this placeholder does not make sense anymore.

How do we handle the case if the ID already exists? I know it seems unlikely but it is possible with this type of GUID generation.

You could create a {{copy-link}} component that does exactly this? It would probably only be a few lines of JS.

I agree, the inviteKey should be generated server side.

I wonder if the + icon to create a new invite, can just make either just the invitekey serverside or maybe it could just create an actual invite with all the defaults. And any changes via the modal popup would just be an update to the invite that was just created.

One thing I noticed playing around this these changes are that after you create a key it doesn’t show up in the pending list until you refresh the page. I think this might be how it worked before when generating a link instead of sending an email, but I think we definitely should fix this now so that a refresh isn’t needed.

I am not so sure about creating an invite when the modal shows up because if a user made a mistake and immediately closes it, then they will have created an invite they did not really need. Maybe I can delete it if the user closes the modal without ever clicking on the create button?

I think if an invite is not found, the route should just return a 404 and let the client side handle it with the right error message.

Should this be moved into conditional above? Otherwise groups is not a defined variable if params[:group_ids] or params[:group_names] is not present.