Refactor Invitation dialog to React and add email preview #7168
Conversation
tomasr8
left a comment
There was a problem hiding this comment.
Looks good overall! I still want to have a deeper look at the InviteDialog but I already left some comments.
One comment for the CSV upload: when it fails (i.e. the CSV is not in a valid format), you can still choose the separator but it has no effect. I'd either disable it or revalidate the file with the new separator.
I'm also not a big fan of the new onPrincipalsResolved logic because now there are two different ways of getting data from that PrincipalList field - one from the final form state and one via this new callback, which seems a bit awkward to me. Maybe instead of onPrincipalsResolved we could have a boolean prop like includeNames or such that would include more details in the field value. So instead of
['User:1', 'User:2']you'd have
[{identifier: 'User:1', firstName: 'John', lastName: 'Doe'}]And then you wouldn't need the callbacks and also getPreviewPayload could also operate only on the values.
indico/modules/events/registration/client/js/components/InviteDialogButton.jsx
Outdated
Show resolved
Hide resolved
my reasoning for that was that, if the upload fails, there's no point in "locking" the separator. why stop the user from changing the separator before reuploading it? it just seems like an unnecessary barrier indeed the ideal behavior would be to revalidate the file, but I don't see a straightforward way to implement that... there's no way to trigger a reupload in EDIT: another solution could be to clear the field when changing the delimiter, but I could also see some users finding that annoying |
fe35524 to
6738a79
Compare
I've been trying to implement this but it really requires reworking the whole principallistfield component... all the handlers, all the subcomponents that provide new principals... |
|
on the phone atm while in a meeting but I really prefer to not make the value of this field heavier... it's identifiers-only on purpose |
6738a79 to
9569df5
Compare
|
@tomasr8 @ThiefMaster I've made some improvements as requested
Thank you again for all the effort looking through this, and I will await further comments 😄 |
a43941d to
feb39d2
Compare
indico/web/client/js/react/components/WTFPrincipalListField.jsx
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/controllers/management/invitations.py
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/controllers/management/invitations.py
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/controllers/management/invitations.py
Outdated
Show resolved
Hide resolved
indico/modules/events/registration/controllers/management/invitations.py
Outdated
Show resolved
Hide resolved
4a4f443 to
32217ee
Compare
32217ee to
0c92ab0
Compare
Otherwise it may be hard to see why sending the emails failed, in cases like inviting someone who's already invited/registered...
| reader = csv.reader(ftxt.read().splitlines(), delimiter=delimiter) | ||
| content = ftxt.read().splitlines() | ||
| if not delimiter and content: | ||
| delimiters = [d.delimiter for d in CSVFieldDelimiter] |
There was a problem hiding this comment.
Is this actually needed? The only thing this avoids is allowing : as a delimiter, but I don't see any reason where this would be an issue here.
| webargs_errors?: UploadErrorMessages; | ||
| } | ||
|
|
||
| function ImportedInvitesMessage({name}: {name: string}) { |
There was a problem hiding this comment.
I think this would be a perfect place to actually show the data from the CSV at some point (not now) instead of just the number.
Otherwise weird data in the first line could result in wrong data
4662b20 to
90b973c
Compare
It's the same but using session.user there is misleading...
This PR adds the ability to preview emails sent in regform invitations. To achieve this, a full refactoring to React was necessary, otherwise it would require adding legacy code to handle obtaining placeholder data for indico user + import invites.
In this refactoring, all three actions (Invite new user, invite existing users, import) are merged into a single dialog:
On top we can select the desired action. By default, invite existing users is selected.
All of the functionality was ported to the new React version.
When importing CSVs, since they are now immediately uploaded, they are verified immediately and show an upload error if there are any CSV format errors. When an upload is successful, the delimiter field is locked and can only be changed if the file is removed.
Future work:
Since pretty much every interactive thing on the invitations page has been reactified now, it seems that the whole page could just become a React component and would simplify things a lot. The only concern is supporting the
registration-invite-optionstemplate hook...