Skip to content
This repository has been archived by the owner on Apr 25, 2023. It is now read-only.

[Blocked] Committee meeting attendees support #842

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

RavivBarzilay
Copy link

@RavivBarzilay RavivBarzilay commented Jul 9, 2017

part of hasadna/knesset-data#143

  • added CommitteeMeetingAttendee model which will store textual parsing result of committee meeting attendees

@OriHoch
Copy link

OriHoch commented Jul 10, 2017

I think some more testing is needed before merging
specifically, to test the other parts of this issue in knesset-data-django hasadna/knesset-data-django#13 - and see if any more changes might be needed for the model

@OriHoch OriHoch changed the title Committee meeting attendees support [Blocked] Committee meeting attendees support Jul 10, 2017
@alonisser
Copy link

Data migration? tests? public api? unicode methods? Without tests this would contribute to technical debt. without public api, not sure what's the use.. And how are the attendants actually added?

Also adding an attendee that is just a text "name" and text "role" and not a Person, is quite problematic in my view. Would be added with different names, no option to add aggregation functionality on a specific person attending. etc..

@OriHoch
Copy link

OriHoch commented Jul 11, 2017

@alonisser - the goal for this is to have data available via re:dash

if you prefer we can also add it in a different module under knesset-data-django, not as part of open-knesset

but, in this case, due to the relationship with committee meeting it made sense to keep it here

@alonisser
Copy link

No problem with keeping it here. Although I do prefer build views or at least an api as an extension task.. redash is very specific.
My main problem is saving Queryable data as a text field Instead of a foreign key to an entity

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants