Skip to content

Manage positions page implementation - #658

Open
conwelld wants to merge 20 commits into
department-portal-basefrom
manage_positions_AD
Open

Manage positions page implementation#658
conwelld wants to merge 20 commits into
department-portal-basefrom
manage_positions_AD

Conversation

@conwelld

@conwelld conwelld commented Jul 22, 2026

Copy link
Copy Markdown

Fixes issue #644

Changes

Created a position route in the main route.
Queried data from the positions history to filter the active positions in history and pass to managepositions.html
Added managepositions.css, where some CSS logic is handled
Added managepositions.js, where the datatable logic is handled
Added logic to preserve allocation privacy
image
image

Testing

It is important to note that cookies for this website will retain the user's role as long as you run it; clear the cookies if you change the user's role. This will make it so that a user should only see the departments they should access when changing the department ID
Worked around the data and UI through inspection
Made sure the passed data is displayed by cross-checking on the database.
changed it to if a user changes the url to another department, th euser can not see other department's allocations if they are not a labor admin

@NYABUTOA
NYABUTOA requested review from MImran2002 and fritzj2 July 22, 2026 19:00
@NYABUTOA NYABUTOA self-assigned this Jul 22, 2026
@conwelld conwelld self-assigned this Jul 22, 2026
@nahom70
nahom70 changed the base branch from development to department-portal-base July 22, 2026 19:39
@nahom70
nahom70 changed the base branch from department-portal-base to development July 22, 2026 19:39
@nahom70
nahom70 changed the base branch from development to department-portal-base July 22, 2026 19:40
Comment thread app/controllers/main_routes/main_routes.py Outdated
Comment thread app/controllers/main_routes/main_routes.py Outdated

if not currentUser.isLaborAdmin:
allowedDepartmentIds = [d.departmentID for d in getDepartmentsForSupervisor(currentUser)]
if dept.departmentID not in allowedDepartmentIds:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to check dept in the returned list directly, without needing an intermediate list, like if dept in getDepartmentsForSupervisor.... Though I'm not 100% sure, as the function returns a modelselect, I think. Really, we do this check enough across all of the new functionality we should probably have an isDepartmentAllowed boolean method that takes a supervisor and department and does this logic internally

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a lot of nuance for this implementation. From what I see, the cookie will "hold" the status, indicating that a user is a labor admin. This can be changed using an incognito window and by changing the debugging user from heggens to pearcej in the secret config. However, this only lets you test the CS department, which could be throwing the 403 error we want, since she does not have access to other departments anyway. I feel the previous implementation we have here works better because it leads down a rabbit hole

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor the department check; use multiple functions with parameters for a query, or an if statement if the query returns anything. Within the getDepartments function.

Comment thread app/static/css/managePositions.css Outdated
Comment thread app/static/css/managePositions.css Outdated
Comment thread app/static/css/managePositions.css Outdated
Comment thread app/static/css/managePositions.css Outdated
userDeptData = request.form
supervisorDeptRecord = SupervisorDepartment.get_or_none(supervisor = userDeptData['supervisorID'], department = userDeptData['departmentID'])
@main_bp.route('/department/<org>/<account>/positions', methods=['GET'])
def managePositions(org, account):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is a newly created departmentPortal.py controller I think anything route that doesn't stays on the three card department page should be in the departmentPortal.py but not mainroute. @BrianRamsay please correct me if I am wrong.

print(f'Could not add user to department: {e}')
return "", 500
if not g.currentUser.isLaborAdmin:
allowedDepartmentIds = [d.departmentID for d in getDepartmentsForSupervisor(g.currentUser)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDepartmentForSupervisor function returns all the departments the user is asscoiated meaning even if they don't exist in the department based on supervisordepartment table they can still see those departments because somewhere they have a lsf assigned to them a form history assigned to them. So we don't want to use that.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

through supervisor department, not just the ones they have a form history for,

if dept.departmentID not in allowedDepartmentIds:
return render_template('errors/403.html'), 403

positions = (PositionHistory.select()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getActivePositions in getpositions.py logic is a great place to either add your logic or refactor the existing getactivepositionlogic to return all position or active position base on parameters.

@MImran2002 MImran2002 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just these and I will be able to merge it

Comment thread app/controllers/main_routes/main_routes.py Outdated
Comment thread app/controllers/main_routes/main_routes.py Outdated
Comment thread app/logic/getPositions.py Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants