Skip to content
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

Viewable reports #357

Merged
merged 4 commits into from
Nov 20, 2018
Merged

Viewable reports #357

merged 4 commits into from
Nov 20, 2018

Conversation

reginafcompton
Copy link
Contributor

This PR handles the last portion of issue #345

Copy link
Collaborator

@hancush hancush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i left a couple of comments inline! one more: if we ever access bills in the ui like lametrobill.objects.all(), it may be a good idea to implement a manager that only returns the viewable bills. i did a similar thing for test events, if we go that route:

https://github.com/datamade/la-metro-councilmatic/blob/64153377330dd1e4b817f53da8a08de04a95313a/lametro/models.py#L160-L172

then,

https://github.com/datamade/la-metro-councilmatic/blob/64153377330dd1e4b817f53da8a08de04a95313a/lametro/models.py#L231-L232

return True

events_with_bill = Event.objects.filter(agenda_items__bill_id=self.ocd_id)
passed_events = [event for event in events_with_bill if (event.status == 'passed' or event.status == 'cancelled')]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this array ever become very large? since we only care about the presence, but not how many, etc., it may be more memory-efficient to do something like:

for event in events_with_bill:
    if event.status in ('passed', 'cancelled'):
        return True
return False

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this list will never be very large – a bill may be associated with one or two events. More than that does not align with what Metro has done historically.

@@ -739,6 +739,8 @@ def search(self):
# Don't auto-escape my query! https://django-haystack.readthedocs.io/en/v2.4.1/searchqueryset_api.html#SearchQuerySet.filter
sqs = sqs.filter_or(attachment_text=Raw(self.cleaned_data['q']))

sqs = sqs.filter(viewable=True)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we always hide bills, should we index them at all?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i.e., do we have all the info we need to omit them when we build the index?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(i forget, does our current pipeline update the index with every scrape, or with enough frequency for this suggestion to make sense?)

Copy link
Contributor Author

@reginafcompton reginafcompton Oct 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked myself this very question!

I decided to index "hidden" bills for one reason – the integrity script, which checks that the Councilmatic database and the Solr index have the same number of bills. If we do not index a "hidden" bill, then this script will complain. I think it's easier to modify the SearchQuerySet, rather than find a workaround with how the integrity script or import_data works.

(N.B. data_integrity runs after every execution of update_index, which runs after import_data which runs four times per hour.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ha, great minds. :-) that makes sense to me. maybe it would be good a drop a comment or docstring in the search_indexes.py file?

@reginafcompton
Copy link
Contributor Author

Ah, that Manager is slick.

I thought about all the places where we render bills, and the only instance where we render all bills is on the search view (which uses the sqs object). For other places, we show bills based on whether or not they have actions (i.e., the person detail) or appear on an agenda (i.e., the event detail).

I do think I need to account for the later case.
For the former case, I do not see how a bill that does not appear on a published agenda could have any actions. Does that sound right?

@hancush
Copy link
Collaborator

hancush commented Oct 18, 2018

@reginafcompton i think that that sounds right, but that a manager may still be a good idea, unless there will ever be a case where we want to show non-viewable bills.

@reginafcompton
Copy link
Contributor Author

@hancush, if we add a context manager, as you've done with the Event, then we'd end up adding the bills to the Solr index (which is not desirable, c.f. above).
Haystack uses the Django ORM to retrieve bills: https://github.com/django-haystack/django-haystack/blob/master/haystack/indexes.py#L154
The _default_manager uses the first manager defined in the class: https://docs.djangoproject.com/en/2.1/topics/db/managers/

I suspect there might be a way to build an LAMetro bill manager, so that Haystack ignores it. But, again, I cannot think of a use case in which we risk showing "hidden" bills (other than the search page)...so, again, I am wondering if we need the manager at all. Do you want to chat about this further?

Copy link
Collaborator

@hancush hancush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approval rescinded pursuant to semaphor convo!

@reginafcompton reginafcompton merged commit 6027703 into master Nov 20, 2018
@reginafcompton reginafcompton deleted the viewable-reports branch November 20, 2018 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants