-
Notifications
You must be signed in to change notification settings - Fork 48
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
Peterson_Fix_Weekly_Summaries_edit_Last_week #2023
Peterson_Fix_Weekly_Summaries_edit_Last_week #2023
Conversation
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.
Hi, I tested your PR and it works as intended. I was able to edit the summary without any errors and the communication between the server and the frontend is happening properly resulting in 200 status code. Refer the video attached for reference.
PR.2023.mov
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.
Hi @peterson337 I reviewed and its working as expected. Able to edit the summary and its saved and remains updated when page is refreshed
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! Overall the code works as expected. However, there is one small bug I encountered. When editing the weekly summaries, if a user tries to save an invalid summary (either blank or less than 50 words), the error will fire as expected but the button will stay disabled. This forces the user to refresh and then make the changes again before being allowed to update.
This can be fixed by just setting the LoadingHandleSave
state to null if an error occurs.
EDIT: Also no need to await setLoadingHandleSave
on line 92 in src/components/Timelog/WeeklySummaries.jsx
video1131734987.mp4
src/actions/weeklySummaries.js
Outdated
@@ -79,7 +79,8 @@ export const updateWeeklySummaries = (userId, weeklySummariesData) => { | |||
|
|||
// Merge the weekly summaries related changes with the user's profile. | |||
const {mediaUrl, weeklySummaries, weeklySummariesCount } = weeklySummariesData; | |||
console.log('respon get', response.data) | |||
|
|||
// console.log('respon get', response.data) |
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.
Remove console.log
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.
c67131d
|
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.
Just an extra set state that is setting the state to the same value. Other than that the changes look good!
} else { | ||
// Invalid summary, show an error message or handle it as needed | ||
alert('Please enter a valid summary with at least 50 words.'); | ||
await setLoadingHandleSave(null); |
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.
Delete the await setLoadingHandleSave
line. Sorry I quoted the wrong code snippet in my other comment
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.
Hi Abi-Liu. I removed the setLoadingHandleSave from the handleSave function.
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.
It's still there. Underneath await dispatch(getUserProfile(userProfile._id));
There's 2 setLoadingHandleSave(null)
Followed all the mentioned steps and the changes look good to me. Those errors didn't appear. Screen.Recording.2024-03-14.at.8.47.06.PM.mov |
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.
Looks good!
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.
This PR works as intended. I was able to edit the summary without any errors and the communication between the server and the frontend is happening properly resulting in 200 status code.
Screen.Recording.2024-03-15.at.5.39.06.PM.mov
There is also a safe check to see if the user edits the summary less than 50 words.
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.
Hello @peterson337
The changes are working as expected. I checked in to current branch and logged in as a user and edited the summary and found no errors in console. It was with 200 ok status.
Thanks
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.
tested the edit function in weekly summaries, and it works perfect without any error in console, and reflect the editing after save correctly.
PR2023.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.
const [editedSummaries, setEditedSummaries] = useState([ | ||
userProfile.weeklySummaries[0]?.summary || '', | ||
userProfile.weeklySummaries[1]?.summary || '', | ||
userProfile.weeklySummaries[2]?.summary || '', | ||
]); | ||
const [originalSummaries, setOriginalSummaries] = useState([...editedSummaries]); | ||
|
||
const [LoadingHandleSave, setLoadingHandleSave] = useState(null); |
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.
// Consider refactoring to use camelCase for consistency everywhere in the codebase.
const [loadingHandleSave, setLoadingHandleSave] = useState(null);
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, I tested your PR and it works perfectly. Below is a video for reference.
Screen.Recording.2024-03-23.at.12.42.06.AM.mov
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.
Hi, I have tested this functionality with its associated backend PR #781, logging in as admin user and volunteer user.
Below screenshots for reference: |
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.
4183794
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.
Tested the PR and there was no error generated while editing summary.
https://github.com/OneCommunityGlobal/HighestGoodNetworkApp/assets/39597927/61df8d27-730b-43df-98c6-e7713b7a27d3
Thank you all, merging! |
1 similar comment
Thank you all, merging! |
Description
In the HGN software, specifically within the 'Tasks and Timelogs' card on the Dashboard, there is a tab named 'Weekly Summaries,' which displays three editable summaries. Upon clicking the 'edit' button below each summary, two additional buttons, 'save' and 'cancel,' appear. The bug was found in the 'save' button functionality, which, when triggered, initiated a 'put' request resulting in two errors. The first error occurred because Node did not receive information from React, resulting in an HTTP status code 500, indicating a server error. The second error was related to the frontend-to-backend request: the request URL should have contained the user ID but was instead sent as 'undefined.'
priority Medium
Related PRS (if any):
This frontend PR is associated with the following backend PR: OneCommunityGlobal/HGNRest#781
Main changes explained:
The WeeklySummaries component has been modified to fix the bugs.
How to test:
npm install
andnpm start
to run this PR locallyScreenshots or videos of changes:
Note:
The console is going to have some errors not related to this PR. These can be disregarded because the code changed by this PR will not introduce these errors to the "development" branch when it gets merged.