-
Notifications
You must be signed in to change notification settings - Fork 6
Group f/development #166
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
Group f/development #166
Conversation
…IPMS into GroupF/accountdeletion
…ount deletion and redirect to homepage
…pF/accountdeletion
Group f/about and contact page
| setSuccessMsg(`Form submitted successfully and sent to ${recipient}`); | ||
| setTimeout(() => setSuccessMsg(""), 15000); | ||
| setFormData(initialState); | ||
| setTimeout(() => { |
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.
Missing cleanup/cancellation of the timeout
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.
Fixed !, added clearTimeout function.
| ? "coordinator for manual review!" | ||
| : "supervisor!"; | ||
| setSuccessMsg(`Form submitted successfully and sent to ${recipient}`); | ||
| setTimeout(() => setSuccessMsg(""), 15000); |
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.
Missing cleanup/cancellation of the timeout. let the team c know about this.
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 just copy pasted this handleSubmit() function from the main branch 3 days ago, no other changes are done by our team. Also, for the setTimeout function that navigates the user back to the student dashboard( that I added), fixed that part by adding the clearTimeout 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.
let team c know sbout other timeout function, so that they can handle 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.
Informed them
package.json
Outdated
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.
why is new package.json added?
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.
those are the existing packages, just the new version updated( bcrypt, dayjs and sweetalert)
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.
why do we need a new one? why cant we add it in the client package.json?
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.
Removed, we actually don't need them.
server/services/emailService.js
Outdated
| }; | ||
| } | ||
|
|
||
| // OPTIONAL SAFEGUARD: Only allow sending if student role |
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.
Why are we not allowing other roles to send emails?
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.
Fixed ! .It was to prevent Token Request emails sending to the supervisor and coordinator, but removed that part and it still emails are not sending to the supervisor/coordinator.
server/services/emailService.js
Outdated
| }; | ||
| } | ||
|
|
||
| // // OPTIONAL SAFEGUARD: Only allow sending if student role |
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.
if this not used just remove 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.
Removed!
package.json
Outdated
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.
why do we need a new one? why cant we add it in the client package.json?
| ? "coordinator for manual review!" | ||
| : "supervisor!"; | ||
| setSuccessMsg(`Form submitted successfully and sent to ${recipient}`); | ||
| setTimeout(() => setSuccessMsg(""), 15000); |
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.
let team c know sbout other timeout function, so that they can handle it.
client/src/pages/Home.js
Outdated
| Remember me | ||
| </label> | ||
| <Link | ||
| {/* <Link |
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 if this is not used
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.
Deleted!
NNPhaniCharan
left a 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.
Resolve conflicts
No description provided.