[WIP] Add online RIGS authorisation #283

tomtom5152 · 2017-04-10T22:37:43Z

Changes from using signed paperwork to an online interface for clients to authorise the rig via a secure HMAC link.

Process is as follows:

MIC provides an authorisation email for RIGS to send the secure link to.
Link is used to identify the client and provide secure access to the event details
Client provides their name, the full amount, has to agree to our Terms of Hire and provide either a PO (external) or Uni ID and Account Code (internal).
Upon successful authorisation, RIGS emails MIC to notify them, and sends a PDF receipt to client.
Details of the authorisation are included on all paperwork once provided, and are superseded by future versions.

This directly closes #228, and indirectly closes #282 and #195.

Merge notes:

migrate RIGS 0024 > merge/deploy > migrate

Add EventAuthorisation model + migrations Add authorised property to Event. Add appropriate tests

Add forms, views, templates and URLs. Remove created at in favour of the built in versioning as that's much more accurate. Switch to a OneToOneField with EventAuthorisation -> event as a result of this. Move validation from models to forms where it probably belongs. Provide more descriptive errors. Add success page for authorisation.

Enable RevisionMixin for EventAuthorisation. Add signal receivers for RIGS. Expand RIGS into an explicitly defined app to support signals.

Add backup email if there isn't an MIC

Add email sending methods. Add TEC side sending of emails.

…RIGS

PDFs now state QUOTE, INVOICE or RECEIPT. Single copy and all but INVOICE includes terms of hire.

…object.

I used python 3 syntax, we aren't yet using python 3...

davidtaylorhq

On the whole I think this looks good. Needs more testing by teccies once treasury approves it though.

Some bugs I noticed:

Confirmation email does not at '£' before cost, and does not add decimal places. e.g. £1140.00 shows as just 1140
Click "Authorisation Request", type an invalid email and submit, it's then impossible to submit again

davidtaylorhq · 2017-04-10T23:41:45Z

RIGS/forms.py

+
+
+class BaseClientEventAuthorisationForm(forms.ModelForm):
+    tos = forms.BooleanField(required=True, label="Terms of hire")


Setting required=true is not enough validation for this field. I can just edit the HTML form to make the checkbox into a text field, and then I don't even have to put any text in the box for it to be accepted. Should be as simple as just adding an if(tos==true) statement to the clean function.

Incorrect, both in testing and according to the docs, required=True on a BooleanField validation ensures that the value=True as well.

True, I suspect the reason this happens is

test = 'no I don't accept' if test: print("They accepted")

But I think it's reasonable to assume clients won't mess with the HTML form, so this is a non-issue

davidtaylorhq · 2017-04-10T23:42:25Z

RIGS/forms.py

+
+    def clean(self):
+        if self.cleaned_data.get('amount') != self.instance.event.total:
+            self.add_error('amount', 'The amount authorised must equal the total for the event.')


I suggest "The amount authorised must equal the total for the event (inc VAT)" for absolute clarity

davidtaylorhq · 2017-04-10T23:45:07Z

RIGS/models.py

+    event = models.OneToOneField('Event', related_name='authorisation')
+    email = models.EmailField()
+    name = models.CharField(max_length=255)
+    uni_id = models.CharField(max_length=10, blank=True, null=True, verbose_name="University ID")


Do SU staff have Uni ID numbers?

Despite even Ollie saying they did, having just asked Niki, they don't. This could be an issue.

davidtaylorhq · 2017-04-10T23:45:23Z

RIGS/rigboard.py

@@ -1,16 +1,18 @@
-import os


Ewww, why was this here!

davidtaylorhq · 2017-04-10T23:46:33Z

RIGS/rigboard.py

-            buffer = StringIO.StringIO()
-
-            buffer = rml2pdf.parseString(rml)
+        context = RequestContext(request, {  # this should be outside the loop, but bug in 1.8.2 prevents this


Remove the now un-necessary comment

davidtaylorhq · 2017-04-10T23:55:41Z

RIGS/templates/RIGS/event_detail.html

-                    {% endif %}
-                {% endif %}
-            </div>
+            {% include 'RIGS/event_detail_buttons.html' %}


I'm glad this has finally been done!

davidtaylorhq · 2017-04-10T23:57:37Z

RIGS/templates/RIGS/event_detail_buttons.html

+        <a class="btn btn-default item-add modal-href event-authorise-request"
+                href="{% url 'event_authorise_request' object.pk %}">
+            <span class="glyphicon glyphicon-send"></span>
+            Authorisation Request


Think we need more context on this button than just colours - otherwise teccies may send many duplicate emails.

This was a tricky one, because by design you can send many emails, this was just to provide feedback that the email had been sent successfully when submitting an address. As it stands RIGS doesn't track when an email has been sent to reduce DB storage.

I think we should track this, waaay too much potential for teccie fail otherwise. It's not like we're short of database shortage.

I'm not against tracking ti for DB storage, wrong phrasing, more complexity of models, which would rapidly grow to have many to many relationships. As I said, current system is stateless, using HMAC. I don't think it would be an issue, but if it is then we can look into it again.

Fair enough. How about sending an email to the teccy when the client gets sent one (obvs without the secret URL)

That would work, but in your scenario the teccie is confused that the email has not been sent, so will just send it again, probably without checking their own email first.

Actually, thinking about this further I did consider not autoclosing the modal and instead displaying a success alert informing the user to do this manually. That would be much more verbose, but also more annoying

An idea I just had: how about just adding "Last sent to", "Last sent at" and "last_sent_by" fields to the EventAuthorisation model. Update those whenever a teccie presses "send", and the history will be kept by reversion

davidtaylorhq · 2017-04-11T00:00:11Z

RIGS/templates/RIGS/eventauthorisation_client_request.txt

+
+{{request.scheme}}://{{request.get_host}}{% url 'event_authorise' object.pk hmac %}
+
+The TEC Rig Information Gathering System


Think this needs a "PLEASE DO NOT RESPOND TO THIS ADDRESS"

I always think that looks god awful, so I've set the reply-to address to something sensible now so theoretically we should never have a problem.

I would also suggest setting rigs@nottinghamtec.co.uk to forward somewhere sensible (productions or it, one or the other)

I'm ok with just setting the reply-to, but just remember if they do reply to the email and it goes to the teccie, the teccie then has the super secret URL

Yep, well aware of that, but at that point, thats their problem for disclosing the secret URL, we have taken reasonable measures already.

davidtaylorhq · 2017-04-11T00:00:17Z

RIGS/templates/RIGS/eventauthorisation_client_success.txt

+Your event is now fully booked and our finance department will be contact to arrange payment.
+{% endif %}
+
+The TEC Rig Information Gathering System


Think this needs a "PLEASE DO NOT RESPOND TO THIS ADDRESS"

davidtaylorhq · 2017-04-11T00:01:40Z

RIGS/templates/RIGS/eventauthorisation_request.html

+
+                    <div class="form-group">
+                        <label class="col-sm-2 control-label"
+                               for="{{ form.email.id_for_label }}">{{ form.email.label }}</label>


This popup needs more warnings.

YOU ARE ABOUT TO SEND THIS TO A CLIENT, QUADRUPLE CHECK ALL THE THINGS, DON'T BE A MORON

The client will be able to see all contact info, event items, the event title, the event description (not notes), and the MIC name.

Do we need to bring out the 'delete invoice' UI for sending emails now?

Preferably yes 🎉

Noticed in testing, that could have gone badly.

… from treasury

…ient

davidtaylorhq · 2017-05-18T16:36:07Z

If it's been sent to the client:

If it's been approved by the client:

# Conflicts: # RIGS/test_functional.py

davidtaylorhq · 2017-05-19T14:53:05Z

Note to self: (cos I don't have time right now)

There's a missing space at the bottom of the paperwork

This effectively reverts 067e03b.

Actually finish fixing PDF footer formatting.

tomtom5152 added 14 commits March 29, 2017 20:46

Add authorisation models.

c2787d5

Add EventAuthorisation model + migrations Add authorised property to Event. Add appropriate tests

Add tests for the client side authorisation

1670b19

Add ID tagging to auth form to autoscroll on error

cf11e82

Add success notification emails.

3b2aa02

Enable RevisionMixin for EventAuthorisation. Add signal receivers for RIGS. Expand RIGS into an explicitly defined app to support signals.

Add test for sending emails.

97b11ea

Add backup email if there isn't an MIC

Add sending of emails to clients.

7fd0c50

Add email sending methods. Add TEC side sending of emails.

Change mic name to use the full name rather than the display name on …

3fa9795

…RIGS

Add tooltip JavaScript

306c11b

Add test for sending authorisation email to client

22119a3

Update templates to include the new authorisation fields

5d17d64

Update PDF templates and enable sending of PDF via email.

391d9ef

PDFs now state QUOTE, INVOICE or RECEIPT. Single copy and all but INVOICE includes terms of hire.

Remove Event.purchase_order in favour of a simple EventAuthorisation …

067e03b

…object.

Fix test client login because we are still using Django 1.8

5be3842

tomtom5152 added enhancement proposal labels Apr 10, 2017

tomtom5152 requested a review from davidtaylorhq April 10, 2017 22:37

davidtaylorhq temporarily deployed to pyrigs-pr-283 April 10, 2017 22:37 Inactive

Fix string formatting issue.

82b6f1c

I used python 3 syntax, we aren't yet using python 3...

tomtom5152 temporarily deployed to pyrigs-pr-283 April 10, 2017 22:44 Inactive

davidtaylorhq reviewed Apr 11, 2017

View reviewed changes

tomtom5152 added 3 commits April 11, 2017 11:45

Add changes suggested by DT

6e78f16

Add useful email addresses for reference.

a0440e1

Add some more disclaimers explaining things better to internal clients.

36638e4

tomtom5152 temporarily deployed to pyrigs-pr-283 April 11, 2017 13:03 Inactive

Add missing PO field.

c0f4884

Noticed in testing, that could have gone badly.

tomtom5152 temporarily deployed to pyrigs-pr-283 April 11, 2017 13:10 Inactive

Few tweaks to printed documents following a conversation with Marilyn…

e12367b

… from treasury

tomtom5152 temporarily deployed to pyrigs-pr-283 April 11, 2017 13:32 Inactive

Add tracking of who sent the link

430862b

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 15:22 Inactive

davidtaylorhq force-pushed the feature/online-auth branch from 88cdbb4 to 843fa7c Compare May 18, 2017 15:30

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 15:30 Inactive

davidtaylorhq force-pushed the feature/online-auth branch from 843fa7c to 99e7878 Compare May 18, 2017 15:34

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 15:34 Inactive

Allow confirmation emails to fail without blocking the interface

b4ab293

davidtaylorhq force-pushed the feature/online-auth branch from 99e7878 to b4ab293 Compare May 18, 2017 15:42

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 15:42 Inactive

Add pound signs to confirmation emails

4e79f00

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 16:23 Inactive

Add warnings when editing an event that has already been sent to a cl…

0a45b04

…ient

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 16:35 Inactive

Merge branch 'master' into feature/online-auth

b6e4c0e

# Conflicts: # RIGS/test_functional.py

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 16:50 Inactive

davidtaylorhq added 2 commits May 18, 2017 18:02

Add failing duplicate test

75a3059

Stop authorisation information being duplicated with an event

4d316c7

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 18, 2017 17:02 Inactive

Change to just using online auth for internal clients.

c6b7bbc

This effectively reverts 067e03b.

tomtom5152 temporarily deployed to pyrigs-pr-283 May 23, 2017 17:19 Inactive

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 23, 2017 17:26 Inactive

davidtaylorhq temporarily deployed to pyrigs-pr-283 May 23, 2017 17:32 Inactive

tomtom5152 added 2 commits May 23, 2017 19:04

Minor PDF styling fixes

d85ebb6

Add payments to invoice PDF

7fdafd8

tomtom5152 temporarily deployed to pyrigs-pr-283 May 23, 2017 18:18 Inactive

Fix text on quote paperwork for external clients

7cb9e97

Actually finish fixing PDF footer formatting.

tomtom5152 deployed to pyrigs-pr-283 May 24, 2017 15:37 View deployment

tomtom5152 merged commit b4ea818 into master May 24, 2017

davidtaylorhq deleted the feature/online-auth branch June 21, 2017 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add online RIGS authorisation #283

[WIP] Add online RIGS authorisation #283

tomtom5152 commented Apr 10, 2017 •

edited

Loading

davidtaylorhq left a comment

davidtaylorhq Apr 10, 2017

tomtom5152 Apr 11, 2017

davidtaylorhq May 18, 2017

davidtaylorhq Apr 10, 2017

davidtaylorhq Apr 10, 2017

tomtom5152 Apr 11, 2017

davidtaylorhq Apr 10, 2017

davidtaylorhq Apr 10, 2017

davidtaylorhq Apr 10, 2017

davidtaylorhq Apr 10, 2017

tomtom5152 Apr 11, 2017

davidtaylorhq Apr 11, 2017

tomtom5152 Apr 11, 2017

davidtaylorhq Apr 11, 2017

tomtom5152 Apr 11, 2017

tomtom5152 Apr 11, 2017

davidtaylorhq Apr 11, 2017

davidtaylorhq Apr 11, 2017

tomtom5152 Apr 11, 2017

davidtaylorhq Apr 11, 2017

tomtom5152 Apr 11, 2017

davidtaylorhq Apr 11, 2017

davidtaylorhq Apr 11, 2017

samozzy Apr 11, 2017

davidtaylorhq Apr 11, 2017

davidtaylorhq commented May 18, 2017

davidtaylorhq commented May 19, 2017



		class BaseClientEventAuthorisationForm(forms.ModelForm):
		tos = forms.BooleanField(required=True, label="Terms of hire")


		{{request.scheme}}://{{request.get_host}}{% url 'event_authorise' object.pk hmac %}

		The TEC Rig Information Gathering System

[WIP] Add online RIGS authorisation #283

[WIP] Add online RIGS authorisation #283

Conversation

tomtom5152 commented Apr 10, 2017 • edited Loading

davidtaylorhq left a 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

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

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

davidtaylorhq commented May 18, 2017

davidtaylorhq commented May 19, 2017

tomtom5152 commented Apr 10, 2017 •

edited

Loading