Closed michaeltorres1 closed 5 years ago
After noticing some issues with my PR I have made a couple of updates...
teacherClasses.html
. I have updated the PR to reflect this change. The date is now shown correctly. After doing some research the added characters at the end of the date were there because by default JavaScript displays the browsers time zone. Because we only want the date to be shown, the date had to be reformatted using a function convertDate
which was already defined in profileController.js
. However, after adding this function to teacherController.js
the above function did not work because in my case it was expecting three parameters which I was able to concluded when trying to use the new Date
constructor. So in order to make it work I had to add in teacherClasses.html
the pipe symbol |
which takes the output on the left, for both startDate
and endDate
, and pipes it to the right.npm run pretest
for linting. After the process, I had fixed the issues and no errors are shown now. As a result I have deleted some variables and modules that were included that were unused such as uniqid
in teacherControlPanel.js
and some console.log
methods that were only needed for testing purposes. Also some other changes were made to comply with the rules stated in the ESLint documentation.Please make sure to check that the date is displayed correctly. Also by running --fix
to automatically fix most of the issues that were found by linting, please make sure that this process did not make any unwanted changes.
Thank you for the update. I am seeing a few things that need to get fixed:
logger.error(
createClasss: \n${error});
above the last general error that logs an unexpected error. This is true for drop class as welljoinClass
and getClasses
for student. The date there is wrong as well. In joinClass
you will also need the following on line 72 since the passwords are now hashed:
if (!authHelpers.compareHashed(req.body.password,result2.password))
teacherControlPanel.js
there is a empty param in comment for dropClass(req, res)
. The | date
for the dates is a pretty cool feature I did not know about. Good find 👍
Thank you for the review @KevinKelly25. I have fixed all the issues besides for bullet point 7 which does not fall into the scope of this PR. I agree we need to fix this soon however in the mean time I will go ahead and make an issue for this.
I went ahead and fix merge conflicts so that there isn't any problems for approving the PR.
Thanks @michaeltorres1. This PR functions well after the issues noted by @KevinKelly25 were corrected. After reviewing the code, I have only one suggestion: when a teacher creates a class through the Add Class
modal, the Join Password
textbox should obfuscate the password. The teacher can always check the password before submitting using the browser supplied "peek" functionality.
Thank you @cinnaco for your observation. I agree that the password should be obfuscated. I went ahead and masked the password so now all you should see is asterisks instead of the actual password.
In this PR I have updated the middleware in order for the
createClass
anddropClass
functions in order for the user to be able to create a class and drop a class through using the interface inteacherClasses.html
. I also have updated some comments and added comments as well. In the functioncreateClass
inteacherControlPanel.js
I have added parameters to reflect what is needed from thecreateClass
function fromclassMgmt.sql
. I have tested this PR and works on my end.Please be sure that your user created has the attribute
isTeacher
totrue
in order to access the classes tab.