Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

leave report: add filter for dept employee appt status #206

Merged

Conversation

jxjj
Copy link
Contributor

@jxjj jxjj commented Sep 13, 2024

ScreenShot 2024-09-13 at 13 04 56@2x

This adds a new filter to the Dept Leaves report, allowing users to choose whether to show only currently active dept employees or all department employees.

UserService::getDeptInstructors() is updated to add a new hasActiveDeptAppointment property to each instructor. The method adds an api request to Bandaid to get the current dept employee list, and then compares this active list with the list of all instructors. We could probably optimize this further (maybe just returning the active status directly from bandaid?) if needed.

On the frontend, the a new onlyActiveAppointments filter is added to the course planning store. By default, this is true – only active instructors will be shown in the report.

One gotcha was with the TA tab – the new 'only active' default seems to prevent any TA's from being shown (presumably because these TA's are not considered dept appointments). As a workaround, if a user clicks the TA tab, the filter will automatically change to "All". It's a little hacky, but simple and might be good enough – we could look at other solutions, tho.

Let me know if any labels or UI elements should change.

On dev.

Known issue: tests need to be updated.

Resolves #204

@jxjj jxjj requested a review from cmcfadden September 13, 2024 18:30
@jxjj jxjj self-assigned this Sep 13, 2024
Copy link
Member

@cmcfadden cmcfadden left a comment

Choose a reason for hiding this comment

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

looks good - is the test failure something to be concerned about?

@jxjj
Copy link
Contributor Author

jxjj commented Sep 13, 2024

update: test fixed. Needed to stub bandaid response to /department/*/employees for one test since we're now calling that endpoint to determine if instructor is current. (Other tests were already including this, but this test clears those to substitute it's own response).

@jxjj jxjj merged commit 196afe7 into develop Sep 13, 2024
2 checks passed
@jxjj jxjj deleted the feature/204-add-control-to-only-show-active-appointments branch September 13, 2024 20:16
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.

Leaves Planing Report - add filter for dept affiliation
2 participants