Refactoring links forms #646
Conversation
dismantl
force-pushed
the
links-refactor
branch
from
937e9bf to
7e148c6
Compare
|
Restored the |
redshiftzero
force-pushed
the
links-refactor
branch
from
3ff9d28 to
f295369
Compare
|
status of this review: rebased on latest |
- add New Link/Video button to officer profiles - add edit and delete buttons to links on officer profiles - add LinksApi for basic CRUD operations on links, including html templates - remove links subform from edit officer page - change user_id to creator_id in links table - add optional officer_id to links table - remove officer_links table - add links to officer profiles in test data
… unit tests for adding/editing/deleting officer links.
|
Ready for review. This PR has been rebased on develop and the issue you mention @redshiftzero should be fixed. I've also added relevant unit tests. |
OpenOversight/app/main/views.py
Outdated
|
|
||
|
|
||
| # This API only applies to links attached to officer profiles, not links | ||
| # attached to incidents. |
There was a problem hiding this comment.
maybe move the code comment as a doc comment inside the class? e.g.
class OfficerLinkApi:
'''This API only....
attached to incidents
'''
| if hasattr(form, 'creator_id') and not form.creator_id.data: | ||
| form.creator_id.data = current_user.get_id() | ||
|
|
||
| if form.validate_on_submit(): |
There was a problem hiding this comment.
Can you add an else to catch if there are errors and log them (or anything other than die silently?), in the spirit of #786 ?
There was a problem hiding this comment.
The ModelView class it inherits from is written in the same way, so I wouldn't want to break that pattern here without doing it elsewhere, to keep things consistent. And I think it would be best to come to consensus on a solution for #786 and apply it in a separate PR.
There was a problem hiding this comment.
LGTM. Code looks good, and poking around right now in the interface. Looks good to both add, remove, and with none/one/many. Add Link+Video is a little weird when they are two different categories, but I think I don't hate the fact that the add button is conjoined. I gave to my partner to poke around too and got a 👍 from them :)
Improvement request: Can we do more in-line form editing rather than as a separate page? That's maybe more of a general thing if nothing else really has that.
Yes I like that idea for inline forms rather than separate pages! Thanks for the review @fritzdavenport! |
There was a problem hiding this comment.
Thanks @dismantl this looks great. Sorry, I seemed to have missed this PR earlier.
One minor thing: Since you changed the field that the advanced-csv-import expects for links, could you also update the field in the documentation: https://github.com/lucyparsons/OpenOversight/blob/develop/docs/advanced_csv_import.rst#links-csv?
dismantl
dismissed stale reviews from abandoned-prototype and fritzdavenport
via
7f87290
dismantl
force-pushed
the
links-refactor
branch
4 times, most recently
from
879a850 to
bcb6b4f
Compare
dismantl
force-pushed
the
links-refactor
branch
from
bcb6b4f to
91e4889
Compare
There was a problem hiding this comment.
Re-approving. And thanks for adding the documentation-changes @dismantl!
Status
Ready for review
Description of Changes
Fixes #574, #611.
Changes proposed in this pull request:
Notes for Deployment
Screenshots (if appropriate)
Tests and linting
I have rebased my changes on current
developpytests pass in the development environment on my local machine
flake8checks pass