-
Notifications
You must be signed in to change notification settings - Fork 77
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
Overhaul the /departments/<department_id>
view
#1164
base: develop
Are you sure you want to change the base?
Conversation
page: int = 1, | ||
race=None, | ||
gender=None, | ||
rank=None, | ||
min_age=None, | ||
max_age=None, | ||
last_name=None, | ||
first_name=None, | ||
badge=None, | ||
unique_internal_identifier=None, | ||
unit=None, | ||
current_job=None, | ||
require_photo: Optional[bool] = None, |
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.
flask was never setting these vars, so it is basically just a variable initializer list
), | ||
url_for("main.list_officer", department_id=department_id) | ||
+ "?" | ||
+ urlencode(request.args.to_dict(flat=False), doseq=True), |
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 doesn't look the cleanest, but unfortunately flask's url_for
does not have a keyword to supply the get arguments separately and this works.
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 seems to work for me:
url_for("main.list_officer", department_id=department_id, **request.args),
Would you mind giving that a try?
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 is what I started with, but if someone puts department_id=
in the url it will cause a server error which I wanted to avoid
This is making better use of the BrowseForm for filtering to avoid a lot of boilerplate code and adds some other improvements like sending fewer sql queries (by avoiding some N+1 issues).
8a7dfc5
to
bf173ea
Compare
img_id_to_officer = {} | ||
for officer in officers.items: | ||
officer_face = sorted(officer.face, key=lambda x: x.featured, reverse=True) | ||
|
||
# Could do some extra work to not lazy load images but load them all together. | ||
# To do that properly we would want to ensure to only load the first picture of | ||
# each officer. | ||
if officer_face and officer_face[0].image: | ||
officer.image = officer_face[0].image.filepath | ||
|
||
choices = { | ||
"race": RACE_CHOICES, | ||
"gender": GENDER_CHOICES, | ||
"rank": [(rc, rc) for rc in rank_selections], | ||
"unit": [(uc, uc) for uc in unit_selections], | ||
} | ||
|
||
next_url = url_for( | ||
if officer.face: | ||
face_image_id = sorted( | ||
officer.face, key=lambda x: x.featured, reverse=True | ||
)[0].img_id | ||
img_id_to_officer[face_image_id] = officer | ||
images = Image.query.filter(Image.id.in_(img_id_to_officer.keys())).all() | ||
for image in images: | ||
img_id_to_officer[image.id].image = serve_image(image.filepath) |
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.
On my end, I'm seeing that many officers who have images available are showing up as "No Photos" in the browse view. This is happening because multiple officers have the same image assigned to them.
I'm not sure that this happens in practice since we usually use face thumbnails but it might be an edge case want to account for.
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, that shouldn't happen in practice, but I agree with the concern and will make adjustments
@@ -91,11 +91,9 @@ <h3 class="panel-title accordion-toggle">Officer Rank, Unit, and Status</h3> | |||
name="rank" | |||
class="select2-multi-search" | |||
multiple="multiple"> | |||
{% for rank_key, rank in choices['rank'] %} |
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.
For awareness, a future version of wtforms may break this:
pallets-eco/wtforms#811
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.
Good call! I will change it so it works for both versions
/departments/<department_id>
view
Description of Changes
I ran into this when working on #1148
The BrowseForm was basically only used as a data-container, this change is making sure we also use it to filter the incoming data to simplify the code in the view file. I also included other improvements like reducing the number of SQL queries sent.
Functionality wasn't changed (at least that was the goal), so the user experience should remain the same.
Notes for Deployment
N/A
Screenshots (if appropriate)
Tests and Linting
develop
branch.make create_db_diagram
command.pytest
passes on my local development environment.pre-commit
passes on my local development environment.