-
Notifications
You must be signed in to change notification settings - Fork 420
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
Allow importing registrations/contributions from CSV files #3144
Conversation
class ImportRegistrationsForm(IndicoForm): | ||
source_file = FileField(_("Source File"), accepted_file_types='.csv') | ||
skip_moderation = BooleanField(_("Skip Moderation"), widget=SwitchWidget(), default=True, | ||
description=_("If enabled, the registration will me immediately accepted")) |
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.
... will be immediately ...
@@ -40,6 +40,7 @@ | |||
from indico.web.forms.base import IndicoForm, generated_data | |||
from indico.web.forms.fields import (EmailListField, IndicoDateTimeField, IndicoEnumSelectField, JSONField, | |||
PrincipalListField) | |||
from indico.web.forms.fields.files import FileField |
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.
can be imported from indico.web.forms.fields
in the statement right above
description=_("If enabled, the registration will me immediately accepted")) | ||
notify_users = BooleanField(_("E-mail users"), widget=SwitchWidget(), | ||
description=_("Whether the imported users should receive an e-mail notification")) | ||
create_pending = BooleanField(_("Match missing users"), widget=SwitchWidget(), default=True, |
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.
Is there any good reason to create pending users here? If a user creates an indico account later, they are associated with the registration anyway, and in the user search we never show pending users. So I'd rather offer to create an EventPerson so they can find him when searching to add e.g. a speaker - but never a pending indico User
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.
And how would we phrase that option (EventPerson)?
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.
there's a reason why i didn't suggest a possible description for this :P
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.
Then I'll just remove the option and set it to False
by default. Anyway, the unification of Event Persons with registrations in version 2.1 should make this irrelevant.
{%- endtrans %} | ||
{% endcall %} | ||
|
||
{{ form_header(form, multipart=True, action=url_for('.registrations_import', regform)) }} |
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.
true
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.
the url is the same as the one the dialog is loaded from so omitting it should work. also, I think you can use simple_form()
here
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.
Dropzone complains If I specify no URL.
{{ form_rows(form) }} | ||
|
||
{% call form_footer(form) %} | ||
<input type="submit" class="i-button big highlight" value="{% trans %}Submit{% endtrans %}" disabled> |
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.
why disabled
? shouldn't this rather be data-disabled-until-change
?
@@ -0,0 +1,30 @@ | |||
{% extends 'layout/base.html' %} |
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.
It's a dialog so this and the block shouldn't be needed
<li>Affiliation</li> | ||
<li>Position</li> | ||
<li>Phone Number</li> | ||
<li>E-mail</li> |
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.
Wouldn't it make more sense to put First/Last/Email first since those are probably the most common fields?
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.
I put 'e-mail' at the end because some of the other fields may be empty (e.g. position), and the resulting CSV file may end up having < 6 columns.
from indico.web.forms.base import IndicoForm | ||
from indico.web.forms.widgets import SwitchWidget | ||
|
||
|
||
class CSVImportException(Exception): |
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.
inherit from UserValueError
so it's not reportable?
543b7bd
to
78eb55d
Compare
|
||
class ImportRegistrationsForm(IndicoForm): | ||
source_file = FileField(_("Source File"), accepted_file_types='.csv') | ||
skip_moderation = BooleanField(_("Skip Moderation"), widget=SwitchWidget(), default=True, |
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.
I'd remove this if moderation is disabled, like we do in InvitationFormBase
try: | ||
first_name, last_name, affiliation, position, phone, email = [to_unicode(value).strip() for value in row] | ||
except ValueError: | ||
raise CSVImportException('Malformed CSV data - please check that the number of columns is correct') |
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.
_(...)
?
with db.session.no_autoflush: | ||
registrations.append(create_registration(regform, { | ||
'email': email, | ||
'first_name': first_name.title(), |
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.
http://www.kalzumeus.com/2010/06/17/falsehoods-programmers-believe-about-names/ ;)
Some of the things there are probably not super relevant, but I have the feeling titlecasing the name will screw e.g. many dutch names ("Foo van Bar" etc.) that have lowercase words. Maybe better to only titlecase if the original value is lowercase-only?
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.
The problem is actually UPPERCASE NAMES.
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.
lowercase-only or uppercase-only then ;)
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.
Well, the question is whether we'd rather have people input all kinds of silly stuff (CAPITALS!) or miss a few ones here and there. I can't see any better solution, unfortunately.
@@ -492,3 +499,25 @@ def generate_ticket(registration): | |||
|
|||
def get_ticket_attachments(registration): | |||
return [('Ticket.pdf', generate_ticket(registration).getvalue())] | |||
|
|||
|
|||
def import_registrations_from_csv(regform, f, skip_moderation=True, notify_users=False): |
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.
s/f/fileobj/
?
Probably good to rebase to v2.1-dev and add a changelog entry |
I'll actually add some tests first. |
e36e2f7
to
ad0900f
Compare
bab2646
to
387e190
Compare
</ul> | ||
<div class="group"> | ||
<a class="i-button js-enable-if-checked arrow disabled" | ||
title="{% trans %}Export data{% endtrans %}" |
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.
wrong indentation
<ul class="dropdown"> | ||
<li> | ||
<a href="#" class="icon-file-pdf js-submit-form js-enable-if-checked disabled" | ||
data-href="{{ url_for('.contributions_pdf_export', event) }}"> |
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.
wrong indentation (also a few more times below)
following order: | ||
<ul> | ||
<li>Start date/time in <a href="https://en.wikipedia.org/wiki/ISO_8601">ISO 8601</a> format</li> | ||
<ul> |
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.
an <ul>
right inside an <ul>
looks wrong
{% from 'message_box.html' import message_box %} | ||
|
||
{% call message_box('highlight', large_icon=true) %} | ||
{% trans -%} |
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.
is it really a good idea to have this all in a single trans block? seems like it could be split easily (one per list item, one for the intro sentence, one for the sentence at the end) without having something that might be messy in other languages
@@ -16,22 +16,29 @@ | |||
|
|||
from __future__ import unicode_literals | |||
|
|||
import csv | |||
import dateutil.parser |
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.
not stdlib
|
||
{% call message_box('highlight', large_icon=true) %} | ||
{% trans num_columns=6 -%} | ||
You should upload a CSV (comma-separated values) file with exactly {{num_columns}} columns in the |
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.
space around num_columns
{% from 'message_box.html' import message_box %} | ||
|
||
{% call message_box('highlight', large_icon=true) %} | ||
{% trans num_columns=6 -%} |
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.
I think this big trans block could also be split into small ones
'affiliation': affiliation, | ||
'phone': phone, | ||
'position': position | ||
}, notify_user=notify_users, management=skip_moderation)) |
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.
Using the management
kwarg also affects e.g. the log realm. Maybe better to add a new skip_moderation
kwarg?
assert 'phone' not in data | ||
|
||
|
||
@pytest.mark.usefixtures('request_context') |
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.
I think we should rather use session.user if session else None
(__bool__
of LocalProxy catches the RuntimeError and returns False in that case) when logging the registration instead of using/requiring a request context. It doesn't make much sense that something as general as create_registration
only works with an active request context...
|
||
|
||
class ImportContributionsForm(IndicoForm): | ||
source_file = FileField(_("Source File"), accepted_file_types='.csv') |
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.
in some places we specify a mime type for accepted_file_types
, in others a file extension... not sure if there's any advantage of using one or the other.
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.
The MIME type wasn't working fine on my browser. I'm not sure OSs set always the same for CSV files.
4b024fc
to
9c1ec83
Compare
c568fc3
to
3c36016
Compare
OK, I believe everything has been fixed. Awaiting final QA. |
if changes: | ||
flash(_("Event dates/times adjusted due to imported data."), 'warning') | ||
return jsonify_data(flash=False, redirect=url_for('.manage_contributions', self.event), | ||
redirect_no_loading=True) |
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.
I don't think redirect_no_loading
is useful here. I added it for actions that trigger a download prompt where you usually remain on the same page and thus don't want to block it with the "Loading" dialog
<li> | ||
{%- trans link='<a href="https://en.wikipedia.org/wiki/ISO_8601">'|safe, endlink='</a>'|safe, | ||
subitem='<ul><li><em>'|safe, endsubitem='</em></li></ul>'|safe -%} | ||
Start date/time in {{link}}ISO 8601{{endlink}} format |
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.
missing space inside {{ ... }}
<ul> | ||
<li> | ||
{%- trans link='<a href="https://en.wikipedia.org/wiki/ISO_8601">'|safe, endlink='</a>'|safe, | ||
subitem='<ul><li><em>'|safe, endsubitem='</em></li></ul>'|safe -%} |
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.
the indentation seems weird
reader = csv.reader(f) | ||
contributions = [] | ||
all_changes = defaultdict(list) | ||
for n_row, row in enumerate(reader, 1): |
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.
row_num
?
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.
😠
72ff4bb
to
40bcee8
Compare
@@ -62,6 +62,7 @@ def _ensure_consistency(contrib): | |||
|
|||
|
|||
def create_contribution(event, contrib_data, custom_fields_data=None, session_block=None, extend_parent=False): | |||
user = session.user if session else None |
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.
is it only for making tests run?
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.
It also lets you use this operation from the Indico shell without creating a request context.
</li> | ||
</ul> | ||
{% trans -%} | ||
Only the field "Title" is mandatory. Users will be matched with existing |
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.
what about E-mail address? isn't it mandatory as well?
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.
No, you may have speakers without an email address.
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.
And no speakers at all 😉
e1f8229
to
39c97be
Compare
For now it only supports personal data fields.
39c97be
to
77f4a15
Compare
Wrapping avoids the dialog being wider than other dialogs
For now it only supports personal data fields (registration) and single speakers (contributions).