Conversation
|
@hrideshmg Could you review this please ? |
naveen28204280
force-pushed
the
leave_tracking
branch
from
d9d2ffe to
f7b758b
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces leave tracking functionality to support the amD Discord bot by creating a Leave table in the database and adding corresponding GraphQL mutation and query operations. The changes enable tracking member leaves with approval workflows and querying leave counts within date ranges.
Changes:
- Created a Leave database table with foreign key references to Member via discord_id, including constraints for leave approval validation
- Added
mark_leavemutation to record new leave entries with discord_id, reason, duration, and optional approver - Added
leave_countquery method to Member type for calculating total leave duration within a date range - Extended the
memberquery to accept discord_id as an optional parameter alongside existing member_id and email options
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 14 comments.
| File | Description |
|---|---|
| migrations/20260214152404_create_leave_table.sql | Creates Leave table schema with foreign keys, defaults, and constraints including self-approval prevention |
| src/models/attendance.rs | Defines MarkLeaveInput, MarkLeaveOutput, and LeaveCountOutput structs for GraphQL API |
| src/graphql/mutations/attendance_mutations.rs | Implements mark_leave mutation to insert leave records with current timestamp |
| src/graphql/queries/member_queries.rs | Adds discord_id parameter to member query and implements leave_count resolver on Member type |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let leave = sqlx::query_as::<_, LeaveCountOutput>( | ||
| r#" | ||
| SELECT discord_id, SUM(duration) AS count | ||
| FROM "Leave" | ||
| WHERE date > $1 | ||
| AND (date + duration) < $2 | ||
| AND discord_id = $3 | ||
| GROUP BY discord_id | ||
| "#, | ||
| ) | ||
| .bind(start_date) | ||
| .bind(end_date) | ||
| .bind( | ||
| self.discord_id | ||
| .as_ref() | ||
| .expect("Leave count needs discord_id"), | ||
| ) | ||
| .fetch_one(pool.as_ref()) | ||
| .await?; |
There was a problem hiding this comment.
The leave_count query will fail when a member has no leaves in the specified date range. The fetch_one method expects exactly one row, but when there are no matching leaves, the GROUP BY query returns zero rows, causing a database error. Consider using fetch_optional and returning a default count of 0 when no leaves are found, or modifying the query to always return a result using COALESCE.
There was a problem hiding this comment.
Once this method is moved into a leave object, this can be refactored to return a simple integer rather than returning both discord_id and count. The former is available in the parent member object.
See: #170 (comment)
There was a problem hiding this comment.
Once this method is moved into a
leaveobject, this can be refactored to return a simple integer rather than returning bothdiscord_idandcount. The former is available in the parent member object.See: #170 (comment)
Wouldn't that be more difficult when you're running an allMembers query ?
This way we can directly compare with discord_id
There was a problem hiding this comment.
This way we can directly compare with discord_id
You can still do that, discord_id in this current implementation is redundant as its one the same level as your leave_count query.
The refactor was suggested because we need to add another records query as well, see the linked comment.
| async fn leave_count( | ||
| &self, | ||
| ctx: &Context<'_>, | ||
| start_date: NaiveDate, | ||
| end_date: NaiveDate, | ||
| ) -> Result<LeaveCountOutput> { |
There was a problem hiding this comment.
leave should be a nested object with two methods under it that return leave_count and records respectively as we need to be able to query on which days the member took a leave.
See the records and updateCount methods under StatusInfo for reference.
This PR
Leaveformark_ leaveleave_countdiscord_idoptional param forMemberobject so amD can query using discord_id itselfThese are currently meant only to be accessed by amD for marking and fetching leaves