feat: resident profile PR 5 add backend points#1102
feat: resident profile PR 5 add backend points#1102CK-7vn merged 20 commits intoCK-7vn/total-refactorfrom
Conversation
79beffe to
f4a6811
Compare
ae9c0c6 to
0a98407
Compare
f4a6811 to
6a09b18
Compare
0a98407 to
605264f
Compare
6a09b18 to
4629e48
Compare
605264f to
7b9c906
Compare
4629e48 to
a881711
Compare
7b9c906 to
fa308b5
Compare
carddev81
left a comment
There was a problem hiding this comment.
@CK-7vn Here are my findings
There is a rebase issue and assume there may be more than this, I didn't search for all off them, just assuming I could point this one out and you would be able to fix all others:
- Within the 'Classes' listing, the grey background is not expanding the full width of the view port and also all the facility filter is not displaying like we discussed on the 'Class Details' PR--just a rebase.
- On next review I'll double check the completed screens to make sure they are in sync with the total-refactor branch in next review.
- 'Attendance Trend' is out of sync with figma, ie. percentages are not rounded--decimals are displayed
- 'Attendance Trend' states 'last 8 weeks ...'--doesn't display Weeks, it is displaying dates.
- No toast notification when resetting password
- After deactivating a resident the resident's profile doesn't fully update to reflect the changes. ie Active enrollments was 2 until I came back to the screen. NOTE that the pill did update automatically to 'Inactive'
- Similar to issue above, after transferring a resident, the resident's profile doesn't fully update to reflect the changes. ie Active enrollments was 2 until I came back to the screen. NOTE: the 'Current' facility did automatically update.
- The details of the attendance--I believe this should display 'Unexcused' rather than 'Absent' to be consistent with how the prototype is marked up. For example, it displays Absent (Excused) and Absent (Unexcused) for when you take attendance. This is why I'm thinking we should use the word 'Unexcused' here.
- A note consisting of more than 25 characters bleeds to the outside of the modal. @calisio's figma doesn't show what happens in this scenario, I would ask her for design suggestions and research best practices and implement the best but of course the least complex approach.
- The attendance csv file is incorrectly named, it contains an underscore at the end of it.
- The attendance csv file does not contain anything other than the header--no data is being populated into it
- Nav bar scrolls off of screen (similiar issue to Class Details)
- Unable to add historical notes, I see the SQL migration file for notes table. But the functionality is missing? Also since you will be modifying and adding this functionality, can you please add all of the standardized columns to the table for auditing purposes.
- created_at
- updated_at
- deleted_at
- create_user_id
- update_user_id
Please make sure these are all added and you can probably remove admin_id and just rely on the associated columns below, also note that update_user_id could be utilized in the future in case we allow editing.
REVIEWER'S NOTES: This was a review of functionality and Figma synchronization. What remains is a follow-up review of the code changes and backend logic to ensure queries are pulling the correct data.
7f77044 to
1fc8fa3
Compare
carddev81
left a comment
There was a problem hiding this comment.
Rebase Checks
On classes page, you fixed the grey background but there is an issue with the scroll now--nav links scrolls off of the page when scrolling
- Toast message does not match when adding a historical note
- Carry over issue from last review screenshot:
- A note consisting of more than 25 characters bleeds to the outside of the modal. @calisio's figma doesn't show what happens in this scenario, I would ask her for design suggestions and research best practices and implement the best but of course the least complex approach.
- Two things to note here for attendance
- in the details modal it states 'Complete attendance record for this class' but only displays 10 attendances--for this resident I am using as a test it should have 35 displayed. @caliso is this correct to only display the latest 10 records or should this be paginated?
- When I click Export to CSV the CSV contains all attendance taken for all classes versus just the class. I believe the functionality should export the CSV for just the class being requested
carddev81
left a comment
There was a problem hiding this comment.
@CK-7vn There are a few additional coding comments along with these below
- When there are many attendance records to display the modal grows larger than the height of the screen and am unable to access buttons. Add pagination to display only so many records to the user at once? @calisio Is adding pagination an option here?
- Figma will display the note 'Medical Appointment' but current PR displays 'Medical App...'
Target Figma
Current
| return rpc.AttendancePercentage | ||
| } | ||
|
|
||
| type UserNote struct { |
There was a problem hiding this comment.
@CK-7vn Nice! I was praying this could be used.
carddev81
left a comment
There was a problem hiding this comment.
Great work on this one!! I went through it thoroughly and everything looks solid. Backend code is clean, it tested well, and ready to go. Thank you so much for your patience through the review rounds, it paid off!
Pre-Submission PR Checklist
Description of the change
Resident-Profile PR #5
Adds the remaining backend-driven data for the Resident Profile page that was deferred
from PRs 1-4:
Schedule field on Active Enrollments:
human-readable format (e.g. "M/W/F 9:00 AM")
Weekly Attendance Trend endpoint:
present/total rate per week
Historical Notes:
Screenshot(s)
This PR
Redesign
Additional context
refactor branch. The sequence is contiguous in the DB (64 → 65 → 66).
flexibility.
first one. If a class has multiple events with different schedules, the first is
displayed.
POST uses validatedAdminRoute with FacilityAdminResolver so only admins at the
resident's facility can add notes.