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

Feature/anvitaa22/127 student attendance display #128

Merged
merged 12 commits into from
Apr 2, 2023

Conversation

anvitaa22
Copy link
Contributor

Administrative Info

Make sure your branch name conforms to: <feature/staging/hotfix/...>/[username]/[Github Issue]-[3-4 word description separated by dashes].

What issue(s) does this branch close? #127

Changes

What changes did you make?

  • added manage classes + class view to student page
  • modified modules and roster so that student cannot edit roster or add module
  • altered api from getSingleUserAttendanceFromClassID to getSingleUserAttendanceFromSessionID
  • made student attendance display page

Testing

How did you confirm your changes worked?

  • ran npm run tests to make sure api was working
  • checked functionality on localhost

Confirmation of Change

Run app on localhost. Make student, teacher and admin account. Make class with student and teacher. View Student attendance page after clicking on manage class in student view.

Copy link
Collaborator

@Anshul-Birla Anshul-Birla left a comment

Choose a reason for hiding this comment

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

Great work, but some changes i noticed

  • The time should have a "-" in between the start and end time
  • The time is not being parsed correctly -> for example, i made a class from 12-1PM, but it shows up as 8-9PM. I think it is probably a UTC conversion issue, so make sure to check that you display the time as local time
  • The buttons on the student page should not have a pointer for the cursor since they cannot be clicked
  • There is a test case that is failing to pass

thank you :)

Comment on lines 54 to 55
const studentAttendance = await api.getSingleUserAttendanceFromSessionID(user.id, sessionId, classId);
setStudentAttendance(studentAttendance);
Copy link
Collaborator

Choose a reason for hiding this comment

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

since parents should be able to access this page for their students, how about we pass an id in via an optional prop. If its set, we get the information of that user, else we get it from the logged in user id

Comment on lines 49 to 53
if (user.role == "Teacher" || user.role == "Admin"){
const attendances = await api.getAttendanceFromSessionID(sessionId, classId);
setAttendance(attendances);
}
else{
Copy link
Collaborator

Choose a reason for hiding this comment

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

commenting for why we are splitting this up based on roles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Attendance objects returned are different. The Teachers and Admin get a list of Attendance objects whereas a student gets a list of SingleUserAttendance objects

Comment on lines 127 to 130
"SELECT e.name, cl.event_information_id, cl.min_level, cl.max_level, cl.rrstring, cl.start_time, cl.end_time, cl.language, u.id as user_id, u.first_name, u.last_name " +
"FROM (((event_information e INNER JOIN classes cl ON e.id = cl.event_information_id) " +
"INNER JOIN commitments ON commitments.event_information_id = e.id) " +
" INNER JOIN users u ON commitments.user_id = u.id) WHERE role = 'Teacher'",
" INNER JOIN users u ON commitments.user_id = u.id)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Anshul-Birla need to change this for the frontend view of the manage classes view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what to do for this

Copy link
Collaborator

Choose a reason for hiding this comment

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

its for me dont worry

models/SingleUserAttendance.ts Outdated Show resolved Hide resolved
models/SingleUserAttendance.ts Outdated Show resolved Hide resolved
Comment on lines 127 to 130
"SELECT e.name, cl.event_information_id, cl.min_level, cl.max_level, cl.rrstring, cl.start_time, cl.end_time, cl.language, u.id as user_id, u.first_name, u.last_name " +
"FROM (((event_information e INNER JOIN classes cl ON e.id = cl.event_information_id) " +
"INNER JOIN commitments ON commitments.event_information_id = e.id) " +
" INNER JOIN users u ON commitments.user_id = u.id) WHERE role = 'Teacher'",
" INNER JOIN users u ON commitments.user_id = u.id)",
Copy link
Collaborator

Choose a reason for hiding this comment

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

its for me dont worry

@Anshul-Birla Anshul-Birla merged commit 81a9b46 into master Apr 2, 2023
@Anshul-Birla Anshul-Birla deleted the feature/anvitaa22/127-student-attendance-display branch April 2, 2023 03:03
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.

2 participants