-
Notifications
You must be signed in to change notification settings - Fork 46
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
Jiayu fix bug manual add lost volunteer time #2319
Jiayu fix bug manual add lost volunteer time #2319
Conversation
… to fix add lost time for person
…manual_add_lost_volunteer_time Merge development
…er_time merge development
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested the PR as owner and able to add the lost time for Project, Person and Team.
The owner can also edit and delete the lost time for Project, Person and Team.
The issue fix for manual add lost volunteer time is fixed for owner!.
Suggestion: Toaster message can be added to show user the success of add/ edit/ delete lost time.
PR2319.+.PR977.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Jiayu,
Thank you for the clear instructions. This works as expected. Attached a video implementation of the feature working successfully.
977+2319.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Great work, but I have observed while adding lost time for team, steps to reproduce:
Add lost time -> select team -> and type a new team name -> it will ask for create new team with the name you have given, while it doesn't make sense to add lost time for a team that has not been created, I'm not sure that option might be at a wrong place, correct me if I'm wrong. As this might not be related to the current PR, I'm approving this, but this should be addressed elsewhere. Thanks
Screen.Recording.2024-06-29.at.4.56.32.PM.mov
Screen.Recording.2024-06-29.at.4.58.04.PM.mov
Screen.Recording.2024-06-29.at.5.03.15.PM.mov
Hi @pavanswaroopl , I believe this is not related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found an issue that might not be related to this PR, hence approving it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the PR as owner and I can successfully add,delete,update lost time for project, person and team.Here is the video implementation of the related functionality
HGN.APP.-.Google.Chrome.2024-06-29.18-21-34.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tested this PR with backend PR 977. i followed the instructions as given and successfully tested this PR . It working as expected and i was able to add, edit and delete the lost time for project, person and Team.
Suggestion :Toaster messages can be added to notify the user of successful additions, edits, or deletions of lost time.
Thank you all, merging! |
Description
Related PRS (if any):
This frontend PR is related to backend PR# 977.
Main changes explained:
src/actions/timeEntries.js
to return the status code for deleting time entries.AddLostTime.jsx
to fix the boolean checking logic and modify request body.EditHistoryModal.jsx
to fix the boolean checking logic and update the request body.How to test:
npm install
andnpm run start:local
to run this PR locallyScreenshots or videos of changes:
Before Changes
After Changes
https://www.loom.com/share/df6c6c8043ad405088c3766672dec080?sid=e4b769b9-4c1a-4245-9f1e-735f2f9c4342