825 Add assignment end date (resign_date) to UI #832
Conversation
There was a problem hiding this comment.
This looks great! Thanks for the work and specifically for going through the tests and updating them and adding additional ones too and for adding some useful validation as well.
I do think we need to change the way the validation-failure case is handled, but definitely open to your suggestions. And for this one and in general, happy to talk about any issues further, especially if you disagree with my opinion
OpenOversight/app/main/views.py
Outdated
| return redirect(url_for('main.officer_profile', officer_id=officer_id)) | ||
| try: | ||
| faces = Face.query.filter_by(officer_id=officer_id).order_by(Face.featured.desc()).all() | ||
| assignments = Assignment.query.filter_by(officer_id=officer_id).all() | ||
| face_paths = [] | ||
| for face in faces: | ||
| face_paths.append(serve_image(face.image.filepath)) | ||
|
|
||
| except: # noqa | ||
| exception_type, value, full_tback = sys.exc_info() | ||
| current_app.logger.error('Error loading officer profile: {}'.format( | ||
| ' '.join([str(exception_type), str(value), | ||
| format_exc()]) | ||
| )) | ||
| return redirect(url_for('main.officer_profile', officer_id=officer_id)) | ||
|
|
||
| return render_template('officer.html', officer=officer, paths=face_paths, | ||
| faces=faces, assignments=assignments, form=form) |
There was a problem hiding this comment.
This is a good find, that we don't present the error message to the user. However this solution is duplicate code which we should avoid whenever we can, since it makes maintenance harder. Also, this results in the officer profile being loaded with the url officer/<id>/assignment/new, which is not a valid url for GET, so if I now share this url with others for example they will get an error message.
I don't think there is an easy & perfect solution to this. We do have the flash command that allows us to show a message on the next page that is loaded, so given that this tool is only accessible to admins and area coordinators, I would say it's okay to just add a flash("Error: "+str(form.errors)) and keep the current redirect. Eventually we would like our forms to send the data in the background to avoid most page-reloads, that would also solve this.
Let me know if this makes sense and if you have other ideas on this
There was a problem hiding this comment.
Okay this makes sense to me and I totally agree if Admins are aware of the flash errors then I think that's fine! Thanks for calling the url problem to my attention, I didn't consider that.
| {% else %} | ||
| Unknown | ||
| {% endif %} |
There was a problem hiding this comment.
I think in the case that the resign_date is missing we should just leave it blank, since a missing end-date can mean that this assignment is still active, or that it is no longer active, but we don't know when it ended. But I am not insisting on this one, if you think Unknown is better then leave it in
There was a problem hiding this comment.
Yea for sure thank you for catching that
| assert officer.assignments[0].star_no == new_star_no | ||
|
|
||
|
|
||
| def test_ac_cannot_edit_officer_outside_their_dept_badge_number(mockdata, client, session): | ||
| def test_ac_cannot_edit_assignemtn_outside_their_dept(mockdata, client, session): |
| assert '2019-01-01' in rv.data.decode('utf-8') | ||
| assert '2019-12-31' in rv.data.decode('utf-8') |
There was a problem hiding this comment.
instead of testing the return values I would prefer a test that checks the database for the entry. For example something like this:
assignment = Assignment.query.filter_by(star_no='1234', officer_id=officer.id).first()
assert assignment is not None
assert assignment.star_date == date(2019, 1, 1)
assert assignment.resign_date == date(2019, 12, 31)
I know we don't do this in a lot of the existing tests, but its gradual improvements I think
There was a problem hiding this comment.
Yea I definitely agree testing the db is also necessary. I think it makes sense to keep the assert '2019-01-01' in rv.data.decode('utf-8') in as well, since that's effectively testing the template code, which was also changed in this PR. Do you agree?
There was a problem hiding this comment.
I agree, good point
| assert officer.assignments[0].star_date == date(2019, 2, 1) | ||
| assert officer.assignments[0].resign_date == date(2019, 11, 30) |
There was a problem hiding this comment.
This is similar to what I mentioned above, nice! A little shorter too, just need to make sure that there are no other assignments to this officer, but that should be fine I think
There was a problem hiding this comment.
Yea, in other tests I was just deleting other assignments as a first step, is that alright? Or do you want me to switch these to filtering for that assignment specifically?
There was a problem hiding this comment.
Eh I think actually calling the assignment and testing its attributes is better code. I'm not including the line assert assignment is not None if that's cool bc I think the following assertions imply that. Is that alright?
There was a problem hiding this comment.
you are right, it is implied. I thought maybe it helps finding the problem if the test later fails, but really the python error message is pretty clear anyways (like AttributeError: 'NoneType' object has no attribute 'star_date'). So yeah, good call skipping that one
egfrank
force-pushed
the
825_assignment_end_date
branch
from
a2b76e8 to
8816908
Compare
|
@abandoned-prototype just made changes in response to the code review and rebased, let me know what else I can do! |
There was a problem hiding this comment.
Thanks @egfrank! I tested the code locally and everything works. And thanks for also addressing all my suggestions. Planning to merge this tomorrow to give other people a change to review as well
Status
Ready for review!
Description of Changes
Fixes #825.
Changes proposed in this pull request: