-
Notifications
You must be signed in to change notification settings - Fork 27
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
Implementation of Elections spec #82
Conversation
@jamesturk given the movement toward separate apps, should spike this PR and submit another after PR #85 is merged? Or I could also make a PR on the split-apps branch, and this could all go up together. |
let's keep these in separate PRs, but yeah I think we should wait to merge until we've decided one way or another on #83- I'll ping the relevant parties to see if they're OK with the plan |
3 similar comments
1 similar comment
@jamesturk This one is ready for review again. Below are some notes on differences since I first put in the PR. Removed inheritance from non-abstract models
Added models
Removed models
|
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.
looks pretty good to me, I wasn't as involved in the OCDEP process so apologies if any of my questions were already covered
opencivicdata/legislative/migrations/* | ||
opencivicdata/legislative/admin/* | ||
opencivicdata/*/migrations/* | ||
opencivicdata/*/admin/* |
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.
👍
'updated_at', | ||
) | ||
# date_hierarchy across relations was added to django 1.11 | ||
if django_version[0] >= 1 and django_version[1] >= 11: |
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'm curious if you have a need for Django < 1.11 support? technically this supports 1.10 right now, but since 1.11 is LTS I'm OK w/ new stuff requiring 1.11
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.
Our package that will depend on python-opencivicdata
currently supports Django>=1.9, but this particular feature isn't super important. Just adds a nicer filter option to this admin for users who are at least at Django 1.11.
@@ -6,4 +6,4 @@ | |||
from .vote import (VoteEvent, VoteCount, PersonVote, VoteSource) | |||
from .event import (Event, EventLocation, EventMedia, EventMediaLink, EventDocument, EventLink, | |||
EventSource, EventParticipant, EventAgendaItem, EventRelatedEntity, | |||
EventAgendaMedia, EventAgendaMediaLink, EventDocumentLink) | |||
EventAgendaMedia, EventAgendaMediaLink, EventDocumentLink) |
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.
this might be a mistake?
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.
Yeah, I'm not sure what happened exactly, as I would not have had any reason to change this file. Looks like the newline at the end of the file was deleted. I just added it back.
max_length=300, | ||
help_text="For preserving the candidate's name as it was of the candidacy." | ||
) | ||
filed_date = models.DateField( |
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.
should this be a fuzzy date or will we always require Y-m-d
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 our case, all of these dates are exact. I think the source for this data in most states will be a form the candidate files with an election authority. I think you'll either have the full date or no date at all.
But maybe we want to allow users to put in their best guess for the date? Or if the default for OCD is to use fuzzy dates unless there's a good reason to be exact, we would be fine with that.
max_length=300, | ||
help_text='Name of the Election.', | ||
) | ||
date = models.DateField( |
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.
this being an exact date makes sense to me
I think we agreed to sideline questions about multi-day elections (e.g. vote-by-mail)
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.
IMO even vote by mail elections will want to be tied to a specific date on which the votes are expected to be counted and the results will be published. But, yeah, a convo for another time when we know users will need to track when ballots can be cast.
@jamesturk so I think the proposed changes are:
Should I go ahead and make either or both of these? |
I don't think I have any further changes, I think we could go ahead and accept this & consider it provisional (so a bit less strict on backwards incompatible changes until it stabilizes) |
Cool! Let's do this. |
This is excellent news. Thrilled to see this merged and released. |
As defined in this draft proposal.