Allow importing registrations/contributions from CSV files #3144

pferreir · 2017-11-08T14:44:21Z

For now it only supports personal data fields (registration) and single speakers (contributions).

mvidalgarcia · 2017-11-08T14:51:25Z

indico/modules/events/registration/forms.py

+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"))


... will be immediately ...

ThiefMaster · 2017-11-08T14:49:42Z

indico/modules/events/registration/forms.py

@@ -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


can be imported from indico.web.forms.fields in the statement right above

ThiefMaster · 2017-11-08T14:51:39Z

indico/modules/events/registration/forms.py

+                                   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,


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

And how would we phrase that option (EventPerson)?

there's a reason why i didn't suggest a possible description for this :P

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.

ThiefMaster · 2017-11-08T14:51:51Z

indico/modules/events/registration/templates/management/import_registrations.html

+        {%- endtrans %}
+    {% endcall %}
+
+    {{ form_header(form, multipart=True, action=url_for('.registrations_import', regform)) }}


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

Dropzone complains If I specify no URL.

ThiefMaster · 2017-11-08T14:52:55Z

indico/modules/events/registration/templates/management/import_registrations.html

+    {{ form_rows(form) }}
+
+    {% call form_footer(form) %}
+        <input type="submit" class="i-button big highlight" value="{% trans %}Submit{% endtrans %}" disabled>


why disabled? shouldn't this rather be data-disabled-until-change?

ThiefMaster · 2017-11-08T14:53:40Z

indico/modules/events/registration/templates/management/import_registrations.html

@@ -0,0 +1,30 @@
+{% extends 'layout/base.html' %}


It's a dialog so this and the block shouldn't be needed

ThiefMaster · 2017-11-08T14:54:13Z

indico/modules/events/registration/templates/management/import_registrations.html

+                <li>Affiliation</li>
+                <li>Position</li>
+                <li>Phone Number</li>
+                <li>E-mail</li>


Wouldn't it make more sense to put First/Last/Email first since those are probably the most common fields?

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.

ThiefMaster · 2017-11-08T14:55:00Z

indico/modules/events/registration/util.py

 from indico.web.forms.base import IndicoForm
 from indico.web.forms.widgets import SwitchWidget


+class CSVImportException(Exception):


inherit from UserValueError so it's not reportable?

ThiefMaster · 2017-11-09T10:17:16Z

indico/modules/events/registration/forms.py

+
+class ImportRegistrationsForm(IndicoForm):
+    source_file = FileField(_("Source File"), accepted_file_types='.csv')
+    skip_moderation = BooleanField(_("Skip Moderation"), widget=SwitchWidget(), default=True,


I'd remove this if moderation is disabled, like we do in InvitationFormBase

ThiefMaster · 2017-11-09T10:17:46Z

indico/modules/events/registration/util.py

+        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')


ThiefMaster · 2017-11-09T10:20:13Z

indico/modules/events/registration/util.py

+        with db.session.no_autoflush:
+            registrations.append(create_registration(regform, {
+                'email': email,
+                'first_name': first_name.title(),


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?

The problem is actually UPPERCASE NAMES.

lowercase-only or uppercase-only then ;)

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.

ThiefMaster · 2017-11-09T10:20:53Z

indico/modules/events/registration/util.py

@@ -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):


s/f/fileobj/?

ThiefMaster · 2017-11-16T15:37:28Z

Probably good to rebase to v2.1-dev and add a changelog entry

pferreir · 2017-11-20T14:00:36Z

I'll actually add some tests first.

ThiefMaster · 2017-11-21T11:07:33Z

indico/modules/events/contributions/templates/management/contributions.html

-                </ul>
+                <div class="group">
+                    <a class="i-button js-enable-if-checked arrow disabled"
+                    title="{% trans %}Export data{% endtrans %}"


wrong indentation

ThiefMaster · 2017-11-21T11:07:55Z

indico/modules/events/contributions/templates/management/contributions.html

+                    <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) }}">


wrong indentation (also a few more times below)

ThiefMaster · 2017-11-21T11:08:42Z

indico/modules/events/contributions/templates/management/import_contributions.html

+        following order:
+        <ul>
+            <li>Start date/time in <a href="https://en.wikipedia.org/wiki/ISO_8601">ISO 8601</a> format</li>
+            <ul>


an <ul> right inside an <ul> looks wrong

ThiefMaster · 2017-11-21T11:09:23Z

indico/modules/events/contributions/templates/management/import_contributions.html

+{% from 'message_box.html' import message_box %}
+
+{% call message_box('highlight', large_icon=true) %}
+    {% trans -%}


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

ThiefMaster · 2017-11-21T11:09:33Z

indico/modules/events/contributions/util.py

@@ -16,22 +16,29 @@

 from __future__ import unicode_literals

+import csv
+import dateutil.parser


ThiefMaster · 2017-11-21T11:17:57Z

indico/modules/events/registration/templates/management/import_registrations.html

+
+{% 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


space around num_columns

ThiefMaster · 2017-11-21T11:18:11Z

indico/modules/events/registration/templates/management/import_registrations.html

+{% from 'message_box.html' import message_box %}
+
+{% call message_box('highlight', large_icon=true) %}
+    {% trans num_columns=6 -%}


I think this big trans block could also be split into small ones

ThiefMaster · 2017-11-21T11:20:41Z

indico/modules/events/registration/util.py

+                'affiliation': affiliation,
+                'phone': phone,
+                'position': position
+            }, notify_user=notify_users, management=skip_moderation))


Using the management kwarg also affects e.g. the log realm. Maybe better to add a new skip_moderation kwarg?

ThiefMaster · 2017-11-21T11:24:03Z

indico/modules/events/registration/util_test.py

+    assert 'phone' not in data
+
+
+@pytest.mark.usefixtures('request_context')


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...

ThiefMaster · 2017-11-21T11:25:09Z

indico/modules/events/timetable/forms.py

+
+
+class ImportContributionsForm(IndicoForm):
+    source_file = FileField(_("Source File"), accepted_file_types='.csv')


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.

The MIME type wasn't working fine on my browser. I'm not sure OSs set always the same for CSV files.

pferreir · 2017-11-21T15:20:36Z

OK, I believe everything has been fixed. Awaiting final QA.

ThiefMaster · 2017-11-21T15:23:32Z

indico/modules/events/contributions/controllers/management.py

+            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)


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

ThiefMaster · 2017-11-21T15:24:11Z

indico/modules/events/contributions/templates/management/import_contributions.html

+        <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


missing space inside {{ ... }}

ThiefMaster · 2017-11-21T15:24:17Z

indico/modules/events/contributions/templates/management/import_contributions.html

+    <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 -%}


the indentation seems weird

ThiefMaster · 2017-11-21T15:24:39Z

indico/modules/events/contributions/util.py

+    reader = csv.reader(f)
+    contributions = []
+    all_changes = defaultdict(list)
+    for n_row, row in enumerate(reader, 1):


mic4ael · 2017-11-23T08:20:25Z

indico/modules/events/contributions/operations.py

@@ -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


is it only for making tests run?

It also lets you use this operation from the Indico shell without creating a request context.

mic4ael · 2017-11-23T08:32:29Z

indico/modules/events/contributions/templates/management/import_contributions.html

+        </li>
+    </ul>
+    {% trans -%}
+        Only the field "Title" is mandatory. Users will be matched with existing


what about E-mail address? isn't it mandatory as well?

No, you may have speakers without an email address.

And no speakers at all 😉

For now it only supports personal data fields.

Wrapping avoids the dialog being wider than other dialogs

mvidalgarcia reviewed Nov 8, 2017

View reviewed changes

ThiefMaster reviewed Nov 8, 2017

View reviewed changes

pferreir force-pushed the wip/registration-import branch from 543b7bd to 78eb55d Compare November 8, 2017 16:31

mic4ael approved these changes Nov 9, 2017

View reviewed changes

ThiefMaster reviewed Nov 9, 2017

View reviewed changes

pferreir force-pushed the wip/registration-import branch 4 times, most recently from e36e2f7 to ad0900f Compare November 20, 2017 14:03

pferreir changed the title ~~Allow importing registrations from a CSV file~~ Allow importing registrations/contributions from CSV files Nov 20, 2017

pferreir changed the base branch from master to v2.1-dev November 20, 2017 14:04

pferreir force-pushed the wip/registration-import branch 2 times, most recently from bab2646 to 387e190 Compare November 20, 2017 18:06

ThiefMaster reviewed Nov 21, 2017

View reviewed changes

pferreir force-pushed the wip/registration-import branch 2 times, most recently from 4b024fc to 9c1ec83 Compare November 21, 2017 14:40

ThiefMaster added this to the v2.1 milestone Nov 21, 2017

pferreir force-pushed the wip/registration-import branch 5 times, most recently from c568fc3 to 3c36016 Compare November 21, 2017 15:19

ThiefMaster approved these changes Nov 21, 2017

View reviewed changes

pferreir force-pushed the wip/registration-import branch 2 times, most recently from 72ff4bb to 40bcee8 Compare November 21, 2017 15:59

mic4ael reviewed Nov 23, 2017

View reviewed changes

pferreir force-pushed the wip/registration-import branch 2 times, most recently from e1f8229 to 39c97be Compare December 11, 2017 16:52

ThiefMaster changed the base branch from v2.1-dev to master January 12, 2018 15:02

pferreir and others added 6 commits January 26, 2018 18:17

Make log usable in case there's no request context

bb4921a

Allow importing registrations from a CSV file

f894c57

For now it only supports personal data fields.

Move EventPerson logic to util func

683efb9

Allow importing contributions from a CSV file

0367503

Add changelog entry

8051179

Fix incorrect dialog title

77f4a15

ThiefMaster force-pushed the wip/registration-import branch from 39c97be to 77f4a15 Compare January 26, 2018 17:25

Improve formatting in import dialogs

1edf0f5

Wrapping avoids the dialog being wider than other dialogs

ThiefMaster merged commit 1edf0f5 into indico:master Jan 26, 2018

pferreir mentioned this pull request Jan 7, 2019

Bulk-import into timetable #1528

Closed

pferreir deleted the wip/registration-import branch February 7, 2019 13:09

		assert 'phone' not in data


		@pytest.mark.usefixtures('request_context')



		class ImportContributionsForm(IndicoForm):
		source_file = FileField(_("Source File"), accepted_file_types='.csv')

		@@ -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

Allow importing registrations/contributions from CSV files #3144

Allow importing registrations/contributions from CSV files #3144

Conversation

pferreir commented Nov 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster Nov 8, 2017 • edited

Choose a reason for hiding this comment

pferreir Nov 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pferreir Nov 8, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThiefMaster commented Nov 16, 2017

pferreir commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pferreir commented Nov 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pferreir commented Nov 8, 2017 •

edited

ThiefMaster Nov 8, 2017 •

edited

pferreir Nov 8, 2017 •

edited

pferreir Nov 8, 2017 •

edited