576/disable remove depts #660

McEileen · 2019-04-25T18:18:24Z

Status

Ready for review.

Description of Changes

Fixes #576.
Changes proposed in this pull request:

Admins can now disable, enable, edit, or delete a department through the departments dashboard.
Admins can navigate to the departments dashboard by clicking on the "departments" link in the top menu.
Area coordinators can disable or enable the department for which they are an AC.

Notes for Deployment

Screenshots (if appropriate)

Photo of departments dashboard

Photo of enable/disable department page for AC

Tests and linting

I have rebased my changes on current develop
pytests pass in the development environment on my local machine
(The one test that fails, lastname_capitalization, has been fixed in another PR that is currently being reviewed.)
flake8 checks pass

McEileen · 2019-04-25T18:22:00Z

Question for the OO team: There is pre-existing capability for admins to create a new department through the /department/new route. Would everyone be in favor of me refactoring this route to move it from main/views.py to auth/views.py and then incorporating it with the DepartmentsAPI? I could add a Create New Department button at the departments dashboard's bottom, which would then take the admin to the Create New Department page.

dismantl · 2019-05-05T23:09:51Z

Question for the OO team: There is pre-existing capability for admins to create a new department through the /department/new route. Would everyone be in favor of me refactoring this route to move it from main/views.py to auth/views.py and then incorporating it with the DepartmentsAPI? I could add a Create New Department button at the departments dashboard's bottom, which would then take the admin to the Create New Department page.

Any reason to move the routes from auth to main? Since adding/editing/deleting departments isn't really an authentication-related task, I think it might be better to keep them in main.

McEileen · 2019-05-06T18:05:43Z

Since an admin or area coordinator would have to authenticated to access these routes, I thought it made sense to place them in auth/views.py.

redshiftzero · 2019-08-24T03:59:59Z

yeah to echo @dismantl while users do need to be authenticated to make these changes, the auth blueprint is pretty much for account management related stuff (e.g. password resets, account validation and all that), the main blueprint is the right place for the functionality you're adding here

McEileen · 2019-08-24T16:10:08Z

@dismantl @redshiftzero What you're both saying about the functionality's placement makes sense. I will move it from auth/views.py to the main views.py and then re-submit the PR. (I'll also fix the merge conflicts aplenty!)

McEileen · 2019-09-02T02:19:57Z

This PR is now ready for review.

redshiftzero

hey i took a quick look at this, mostly I have a question inline about the expected functionality of the disabling department feature

redshiftzero · 2019-09-02T05:32:45Z

.gitignore

@@ -86,6 +86,7 @@ vagrant/puppet/.tmp

 # Editor files
 .idea/
+.vscode/


duplicate? (see line 92 below)

redshiftzero · 2019-09-02T05:37:51Z

OpenOversight/app/main/views.py

+@ac_or_admin_required
+def view_ac_dept():
+    dept_id = current_user.ac_department_id
+    dept = Department.query.filter_by(id=dept_id).first()


first() -> one() since we only expect one match

👍 It makes sense to use one() if we only expect one match.
Is there ever a situation where a user could be an area coordinator for more than one department? For example, are any LPL members area coordinators for more than one department?

ah, so right now a user can only be an AC of a single department (ref)

redshiftzero · 2019-09-02T05:54:36Z

OpenOversight/migrations/versions/7343a53178db_.py

+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.add_column('departments', sa.Column('is_active', sa.Boolean(), nullable=True))


seems like as part of this migration we should populate this field with is_active=True for existing departments?

redshiftzero · 2019-09-02T05:58:19Z

OpenOversight/app/main/views.py

+def view_ac_dept():
+    dept_id = current_user.ac_department_id
+    dept = Department.query.filter_by(id=dept_id).first()
+    form = ChangeDepartmentStatusForm(is_active=dept.is_active)


actually I have a higher level question about this PR - my expectation is that disabling a department means that:

it won't show up as a department for users to browse or search unless they are the AC of that department (perhaps preparing their collection prior to enabling it) or the admin

links to officers, tags, etc. in that department for users that are not the AC of that department or an admin should 404

right now it looks like in this PR we're toggling a field (is_active) in the database but not using that field anywhere (other than displaying its value in the views available to ACs or admins) - let me know if i'm misunderstanding

redshiftzero · 2019-09-02T06:04:37Z

OpenOversight/app/models.py

@@ -33,9 +33,10 @@ class Department(db.Model):
    name = db.Column(db.String(255), index=True, unique=True, nullable=False)
    short_name = db.Column(db.String(100), unique=False, nullable=False)
    unique_internal_identifier_label = db.Column(db.String(100), unique=False, nullable=True)
+    is_active = db.Column(db.Boolean, default=True)


I think we don't want this to be nullable, since a department is either active or disabled

McEileen · 2020-01-21T04:17:08Z

Hey @redshiftzero and @r4v5,

Here is my (heavily updated!) work on implementing restrictions around active/inactive departments. I had two specific points I wanted to ask about.

Currently, when a user who doesn't have permissions to view an inactive department navigates to that department's list_officer page, they receive a 403. Meanwhile, the same user will receive a 404 error if they navigate to the list_officer page for a department that doesn't exist.

These are technically the correct error codes, but I worry that the 403 could indicate to potential bad actors that a new department is being added to OO. Are you fine with me using 404 in the first situation?

My second question is bigger. I had a hard time figuring out how to respect active/inactive restrictions and area coordinator privileges when working with FlaskForms that use query selectors.

For example, when the AC of an inactive department navigates to the /find route, I would like them to be able to select their inactive department from the dropdown. If I were passing the departments that the AC can view to the template as a discrete argument, I could find all active departments, append the AC's inactive department to the list, and and pass that list to the template. (You can see an example of this kind of logic in lines 67-71 of main/views.py.)

But, since the FlaskForm for the FindOfficerForm uses a query selector, I'm not sure how to provide the form with the correct departments.

I had the same problem when working with the FindOfficerIDForm() for the get_ooid route, the AddImageForm() for the submit_data route, and the ChangeDefaultDepartmentForm() for the change-dept route. Any ideas how to get through this?

Thanks,
Eileen

redshiftzero · 2020-06-23T02:07:09Z

Currently, when a user who doesn't have permissions to view an inactive department navigates to that department's list_officer page, they receive a 403. Meanwhile, the same user will receive a 404 error if they navigate to the list_officer page for a department that doesn't exist.

This is OK, since the user won't learn which department it is since the slug is primary key 👍

But, since the FlaskForm for the FindOfficerForm uses a query selector, I'm not sure how to provide the form with the correct departments.

I had the same problem when working with the FindOfficerIDForm() for the get_ooid route, the AddImageForm() for the submit_data route, and the ChangeDefaultDepartmentForm() for the change-dept route. Any ideas how to get through this?

So it looks like we can pass a query in the view function to the QuerySelectField, from the docs:

The query property on the field can be set from within a view to assign a query per-instance to the field. If the property is not set, the query_factory callable passed to the field constructor will be called to obtain a query.

So if the user is logged in, we can define a query in the view functions where we use the FindOfficerForm to get all enabled departments plus any that they are AC or admin of that are disabled (for non logged in users we'll just use the existing query_factory).

McEileen · 2020-06-25T03:49:22Z

Thanks, @redshiftzero. I will revisit this PR and work on passing a query in the view function to the QuerySelectField.

…depts

McEileen · 2020-10-20T02:34:10Z

This PR is now ready for re-review.

McEileen force-pushed the 576/disable-remove-depts branch from e58a6b8 to 008c800 Compare September 2, 2019 01:48

redshiftzero requested changes Sep 2, 2019

View reviewed changes

redshiftzero mentioned this pull request Sep 2, 2019

Add officer collections that include officers not associated with one of our tracked departments #690

Open

McEileen force-pushed the 576/disable-remove-depts branch 2 times, most recently from 5e8e4f7 to a1fe164 Compare January 21, 2020 02:04

McEileen force-pushed the 576/disable-remove-depts branch from a1fe164 to 82f7edc Compare January 21, 2020 04:17

McEileen force-pushed the 576/disable-remove-depts branch from 82f7edc to c0c4a6b Compare October 2, 2020 17:08

implement active/inactive restrictions for depts

d459860

McEileen force-pushed the 576/disable-remove-depts branch from c0c4a6b to 2f903c3 Compare October 3, 2020 21:21

set query in views for QuerySelectFields affected by active/inactive …

78ff915

…depts

McEileen force-pushed the 576/disable-remove-depts branch from 2f903c3 to 78ff915 Compare October 17, 2020 22:21

McEileen added 2 commits October 18, 2020 16:52

finetune Edit User and Edit Department functionality

741ee8b

refactor department routes

13e6a7d

abandoned-prototype requested a review from redshiftzero October 20, 2020 05:54

Merge branch 'develop' into 576/disable-remove-depts

3827849

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

576/disable remove depts #660

576/disable remove depts #660

McEileen commented Apr 25, 2019

McEileen commented Apr 25, 2019

dismantl commented May 5, 2019

McEileen commented May 6, 2019

redshiftzero commented Aug 24, 2019

McEileen commented Aug 24, 2019

McEileen commented Sep 2, 2019

redshiftzero left a comment

redshiftzero Sep 2, 2019

redshiftzero Sep 2, 2019

McEileen Nov 9, 2019

redshiftzero Nov 10, 2019

redshiftzero Sep 2, 2019

redshiftzero Sep 2, 2019

redshiftzero Sep 2, 2019

McEileen commented Jan 21, 2020

redshiftzero commented Jun 23, 2020

McEileen commented Jun 25, 2020

McEileen commented Oct 20, 2020

576/disable remove depts #660

Are you sure you want to change the base?

576/disable remove depts #660

Conversation

McEileen commented Apr 25, 2019

Status

Description of Changes

Notes for Deployment

Screenshots (if appropriate)

Tests and linting

McEileen commented Apr 25, 2019

dismantl commented May 5, 2019

McEileen commented May 6, 2019

redshiftzero commented Aug 24, 2019

McEileen commented Aug 24, 2019

McEileen commented Sep 2, 2019

redshiftzero left a comment

Choose a reason for hiding this comment

redshiftzero Sep 2, 2019

Choose a reason for hiding this comment

redshiftzero Sep 2, 2019

Choose a reason for hiding this comment

McEileen Nov 9, 2019

Choose a reason for hiding this comment

redshiftzero Nov 10, 2019

Choose a reason for hiding this comment

redshiftzero Sep 2, 2019

Choose a reason for hiding this comment

redshiftzero Sep 2, 2019

Choose a reason for hiding this comment

redshiftzero Sep 2, 2019

Choose a reason for hiding this comment

McEileen commented Jan 21, 2020

redshiftzero commented Jun 23, 2020

McEileen commented Jun 25, 2020

McEileen commented Oct 20, 2020