New filters, sorting, and more updates for browse view #835
Conversation
…ter Card, Google Breadcrumbs
…ields in browse view
…ame.html partial; Allow filtering by both first name and last name
…ic from list_officers.html -> view; remove officer_name.html partial template
… capitalized in the database instead.
…k, total pay, salary, overtime
…th null overtime_pay
…rsight into list-officer-updates
…ports "NULLS LAST" syntax
There was a problem hiding this comment.
Thanks @dismantl, this looks awesome! Really appreciate all the tests you added to make sure the new features stay functional. Sorry it took a little while for me to get back to you with a review. I pointed out a few things that stood out to me, let me know if you disagree with anything.
Looking forward to having these nice features in production soon!
| unique_internal_identifier=form_data['unique_internal_identifier'], unit=form_data['unit']) | ||
| def gen_pagination_url(page): | ||
| return url_for('main.list_officer', department_id=department.id, | ||
| page=officers.next_num, order=order, race=form_data['race'], gender=form_data['gender'], |
There was a problem hiding this comment.
pretty sure this should be page=page instead of page=officers.next_num
| @@ -176,12 +176,12 @@ class Salary(BaseModel): | |||
| officer_id = db.Column(db.Integer, db.ForeignKey('officers.id', ondelete='CASCADE')) | |||
| officer = db.relationship('Officer', back_populates='salaries') | |||
| salary = db.Column(db.Numeric, index=True, unique=False, nullable=False) | |||
| overtime_pay = db.Column(db.Numeric, index=True, unique=False, nullable=True) | |||
| overtime_pay = db.Column(db.Numeric, index=True, unique=False, nullable=False, server_default='0') | |||
There was a problem hiding this comment.
Generally NULL represents missing data so I don't think we should disallow it, because there is definitely a chance that we have salary data without overtime payment information for some departments. Was this needed to make any lookups easier/better defined?
| if order == 0: # Last name alphabetical | ||
| officer_query = officer_query.order_by(Officer.last_name, Officer.first_name, Officer.id) | ||
| elif order == 1: # Rank | ||
| officer_query = officer_query.order_by(nullslast(Job.order.desc())) | ||
| elif order == 2: # Total pay | ||
| officer_query = officer_query.order_by(nullslast(desc(salary_subq.c.salaries_salary + salary_subq.c.salaries_overtime_pay))) | ||
| elif order == 3: # Salary | ||
| officer_query = officer_query.order_by(nullslast(salary_subq.c.salaries_salary.desc())) | ||
| elif order == 4: # Overtime pay | ||
| officer_query = officer_query.order_by(nullslast(salary_subq.c.salaries_overtime_pay.desc())) |
There was a problem hiding this comment.
Two comments here: Firstly, we need to make sure that whatever order we choose there cannot be a tie. So for example rank alone is bad. The reason is that the sorting is not guaranteed to be stable and ties will lead to issues with the pagination. Adding Officer.id as the last field to sort on in all cases would fix it. See #827 for more details on that issue
The second thing is the "magic" numbers, I would prefer to see an Enum here maybe, or strings like last_name, rank, total_pay instead of integers. This one is definitely not a requirement for me to approve this PR though
| RUN wget https://www.sqlite.org/2020/sqlite-autoconf-${SQLITE3_VERSION}.tar.gz && \ | ||
| tar -xzf sqlite-autoconf-${SQLITE3_VERSION}.tar.gz && \ | ||
| cd sqlite-autoconf-${SQLITE3_VERSION} && \ | ||
| ./configure && make && make install && ldconfig && \ | ||
| cd .. && rm -r sqlite-autoconf-${SQLITE3_VERSION} |
There was a problem hiding this comment.
Nice, great addition! This enables us to always have the most recent version of sqlite (by incrementing the SQLITE3_VERSION variable).
| if (window.location.href.includes('order=')) { | ||
| window.location.href = window.location.href.replace(/order=[^&]+/, 'order=' + this.value) | ||
| } else { | ||
| if (window.location.href.includes('?')) { | ||
| window.location.href = window.location.href + '&order=' + this.value; | ||
| } else { | ||
| window.location.href = window.location.href + '?order=' + this.value; | ||
| } | ||
| } |
There was a problem hiding this comment.
This works! If we want to simplify the code we could update it to use URLSearchParams as pointed out here: https://stackoverflow.com/a/41542008 Seems to be supported by everything except IE now.
Status
Ready for review
Description of Changes
Fixes #798, #752, #834.
Changes proposed in this pull request:
filter_by_form()function now takes an optionalorderparameter that will sort the resultsNotes for Deployment
This requires a minor database migration.
Screenshots (if appropriate)
First name filter
Photo availability and total pay filters
First/Last paging buttons and sorting dropdown
Sorting dropdown expanded
Tests and linting
I have rebased my changes on current
developpytests pass in the development environment on my local machine
flake8checks pass