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

big merge in of django-councilmatic views and functionality #392

Merged
merged 12 commits into from
Dec 20, 2023
Merged

Conversation

derekeder
Copy link
Member

@derekeder derekeder commented Dec 15, 2023

Switches to the minimal branch of django-councilmatic which only contains models.

This PR brings in the pieces we had in django-councilmatic views, templates and static assets

Also made a few other updates:

  • removed unused widget templates
  • fixed a style issue showing a double overflow scrolls for PDF viewer
  • setting context variables in templates that were being covered by councilmatic_core.views.city_context that was removed
  • shortened view names
  • consolidated the councilmatic and chicago directories into just the chicago directory

Issues:

@derekeder
Copy link
Member Author

working on getting the search form working again. i believe because we merged our haystack_indexes into one file, the stored index on the live site is no longer valid and is returning this error:

Text fields are not optimised for operations that require per-document field data like aggregations and sorting, so these operations are disabled by default. Please use a keyword field instead. Alternatively, set fielddata=true on [bill_type] in order to load field data by uninverting the inverted index. Note that this can use significant memory.

Scraping some local bills and testing rebuilding the index locally

@derekeder
Copy link
Member Author

derekeder commented Dec 19, 2023

@fgregg ok search issues are resolved. we didn't have to rebuild the index after all. haystack was expecting the SearchIndex to be defined in search_indexes.py so renaming the file resolved it.

This is ready for your review.

# faceted = True creates a keyword field instead of a text field for full
# text searches. By default, text fields cannot be used for faceting or
# sorting in ElasticSearch.
class BillIndex(indexes.SearchIndex, indexes.Indexable):
Copy link
Member Author

Choose a reason for hiding this comment

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

merged with django-councilmatic's haystack-indexes.py

@@ -123,14 +123,13 @@
"django.template.context_processors.request",
"django.contrib.auth.context_processors.auth",
"django.contrib.messages.context_processors.messages",
"councilmatic_core.views.city_context",
Copy link
Member Author

@derekeder derekeder Dec 19, 2023

Choose a reason for hiding this comment

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

this context allowed for a number of settings variables to be universally available in templates. we are now adding them to each view context as needed

@@ -344,12 +334,6 @@
"committee-on-zoning-landmarks-and-building-standards": "The Committee on Zoning, Landmarks and Building Standards shall have jurisdiction over all zoning matters and the operation of the Zoning Board of Appeals and the office of the Zoning Administrator; land use policy generally and land use recommendations of the Chicago Plan Commission and the Department of Planning and Development; building code ordinances and matters generally affecting the Department of Buildings; and designation, maintenance and preservation of historical and architectural landmarks. The Committee shall work in cooperation with those public and private organizations similarly engaged in matters affecting landmarks.", # noqa
}

ABOUT_BLURBS = {
Copy link
Member Author

Choose a reason for hiding this comment

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

setting these in the templates themselves

@@ -0,0 +1,209 @@
from django import template
Copy link
Member Author

Choose a reason for hiding this comment

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

merged with chicago_extras.py

@derekeder derekeder requested a review from fgregg December 19, 2023 21:56
@@ -178,7 +179,7 @@ html {
}

body {
height: 100%;
height: 99%;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the only substantive change here - lowering this prevents two overflow scroll bars from showing. all other changes are from eslint

@derekeder derekeder requested a deployment to chi-councilm-minimal-y64murymy December 20, 2023 15:49 Abandoned
@derekeder derekeder temporarily deployed to chi-councilm-minimal-y64murymy December 20, 2023 15:50 Inactive
@derekeder derekeder temporarily deployed to chi-councilm-minimal-hsfjmr7ln December 20, 2023 15:52 Inactive
@derekeder derekeder temporarily deployed to chi-councilm-minimal-p5oxtlnhj December 20, 2023 16:44 Inactive
@derekeder derekeder temporarily deployed to chi-councilm-minimal-p5oxtlnhj December 20, 2023 16:58 Inactive
@@ -1,9 +1,7 @@
#!/bin/bash
set -euo pipefail

python manage.py migrate --noinput
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we want these things to happen if there is a database?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe i could come up with a better variable, but i want to add a flag just to the review apps and have it skip these steps if present, otherwise do the normal release procedure.

i removed these for now to make sure i don't mess up the live database before i'm confident in the logic

Copy link
Member Author

Choose a reason for hiding this comment

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

we're now using the presence of a PRODUCTION env variable to determine if we run the management commands or not.

if [ -n "${PRODUCTION}" ]; then

chicago/feeds.py Outdated
class ChicagoBillDetailActionFeed(BillDetailActionFeed):
title_template = "feeds/chicago_bill_actions_item_title.html"

class ChicagoCouncilmaticFacetedSearchFeed(Feed):
Copy link
Contributor

Choose a reason for hiding this comment

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

one thing that might be nice to do is simplify out class names.

This could just be FacetedSearchFeed since there's no other classes out there we need to worry about colliding with.

This is a pervasive change, but it might be nice to go ahead and do now?

@derekeder derekeder temporarily deployed to chi-councilm-minimal-p5oxtlnhj December 20, 2023 17:03 Inactive
@derekeder derekeder temporarily deployed to chi-councilm-minimal-p5oxtlnhj December 20, 2023 17:45 Inactive
@derekeder derekeder temporarily deployed to chi-councilm-minimal-p5oxtlnhj December 20, 2023 17:59 Inactive
Copy link
Contributor

@fgregg fgregg left a comment

Choose a reason for hiding this comment

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

I clicked around and it looks good to me!

@derekeder
Copy link
Member Author

ok lets bring it in!

@derekeder derekeder merged commit cb61909 into main Dec 20, 2023
1 check passed
Copy link

sentry-io bot commented Dec 22, 2023

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ AttributeError: 'datetime.date' object has no attribute 'tzinfo' /person/{slug}/rss/ View Issue

Did you find this useful? React with a 👍 or 👎

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.

set up review apps and read from the production database remove django-councilmatic as a dependency.
2 participants