-
Notifications
You must be signed in to change notification settings - Fork 41
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
WriteInPublic views #2345
WriteInPublic views #2345
Conversation
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 left a small comment inline about leaving a comment :)
I think my bigger concern here is how we configure which WriteInPublic instance to use, and at the moment there's just one possible that's specified in general.yml. I think I've mentioned this before, but I think it's very likely we'll want to support using multiple WriteInPublic instances from Django. (Won't the committees be a second instance, for example?)
I haven't thought this through completely, but what came to my mind is that there could be a model in this new app that represents a particulary combination of WriteInPublic URL and instance ID, with username and API key. Then the ID of that model could be part of the URLs defined by this app, so you have separate URLs for using each instance.
I'm a bit conflicted about suggesting this because I know we want to get this change out of the door as soon as possible, but if we want to move to this way of supporting multiple write-it instances shortly, then it'll be a pain having to redirect the old URLs that just assume a single instance, for example.
What do you think?
pombola/writeinpublic/forms.py
Outdated
self.fields['persons'].queryset = Person.objects.filter( | ||
identifiers__scheme='everypolitician', | ||
identifiers__identifier__isnull=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.
I think I'd add a comment here explaining why you're setting the queryset dynamically. (I think I see why, but I had to think about it quite hard!)
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've added a comment about this in 0495020.
@mhl and I discussed this on a call, we decided that in the interests of getting this merged we should leave it as it is for now, and make working with multiple instances something to worry about next week. @mhl has kindly offered to write up an issue about this so it doesn't get forgotten about. |
One other thing - I've noted what you've said about the lack of tests, and that's fine for this week, but it'd be good to come back to that too. |
This is a multi-step form wizard that walks a user through the steps of creating a new message to a representative. It uses django-formtools to roughly replicate the process of writing to a representative as it currently exists on writeinpublic.com. Once the user has written a message it uses the WriteInPublic API to send the message and then takes the user to a confirmation page.
We want to be able to display messages from a WriteInPublic instance on the site. In order to do this we need to retrieve messages from the WriteInPublic API and then display them alongside the metadata for the message.
This uses the WriteInPublic API to pull back messages that have been written to a specific person and displays them on a page. This view isn't actually hooked into a URL yet, but the intention is to AJAX it into the person profile page, I'll do the changes for that in a separate commit/PR.
0495020
to
570a3a1
Compare
This adds the three main views for the WriteInPublic integration:
Note: This doesn't include the
urls.py
entries to actually make these appear in the web interface, I've left these off so we can get this reviewed and merged without actually "launching" the WriteInPublic integration quite yet. Theurls.py
bits are in #2347.I realise that this is currently lacking tests 😞 I would like to remedy that in the near future, but in the interests of getting this launched by Friday I think it's better to get something in place now, then w can iterate on it and add tests etc.
Extracted from #2329
Part of #2326