Admin feature fixes, new admin privilege manager #448
Conversation
…ending/reported - length feature implemented too
…apping to class names
qiandrewj
changed the title
| if (response.status === 200) { | ||
| setAdmins(result) | ||
| } else { | ||
| console.log('Error at getAdmins') |
There was a problem hiding this comment.
Ideally shouldn't be printing to console in the frontend
| * @param _netId the user's net id | ||
| */ | ||
|
|
||
| function addAdminByNetId(_netId: string) { |
There was a problem hiding this comment.
Great work with this new feature! It'll help us better manage admin access semester-to-semester. I've left a couple comments with things I noticed while testing.
Currently, adding the admin by netid doesn't automatically update the frontend - perhaps use useEffect()? I think the remove function also doesn't automatically update.
Also, it'd be nice to attach the admin's full name before the netid (should just be a quick query to the name in the database)
I'm not sure if this is feasible, but restricting who can manage the admins would be great (maybe just permissions for TPM). Along with this, making sure that the admin manager shouldn't be able to remove themselves or add duplicates of other members on the team will help prevent "lockouts" and access issues.
| <AdminUser | ||
| user={admin} | ||
| token={token} | ||
| removeHandler={removeAdmin} |
There was a problem hiding this comment.
For the admin manager, perhaps don't show the remove handle so that they don't accidentally remove themselves
| <h2>Diagnostic information</h2> | ||
| <div className = {styles.stats}> | ||
| <button className={styles.downloadButton} onClick={downloadCSVFile}> | ||
| Download ApprovedReviewCount by Class |
There was a problem hiding this comment.
I would make this a link (like this) to make it clearer that a file can be downloaded from this text.
| /** | ||
| * Gets CSV text string of all reviews that are approved, sorted by class. | ||
| * | ||
| * @param {Auth} auth: Object that represents the authentication of a request being passed in. | ||
| * @returns CSV text if operation was successful, null otherwise | ||
| */ |
There was a problem hiding this comment.
Love all the detailed documentation you added in this file!
| /* | ||
| * If there is an attempt to grant admin privilege to someone not in the database, | ||
| * a new user will be created with the given netid and added to the database. | ||
| */ | ||
|
|
||
| export const createNewAdminUser = async (id: string) => { | ||
|
|
||
| const admin: InsertStudentType = { | ||
| _id: shortid.generate(), | ||
| firstName: '', | ||
| lastName: '', | ||
| netId: id, | ||
| affiliation: '', | ||
| token: '', | ||
| privilege: 'admin', | ||
| }; | ||
|
|
||
| const newAdmin = new Students(admin); | ||
| const res = await newAdmin.save(); | ||
|
|
||
| return res | ||
| } |
There was a problem hiding this comment.
There should probably be some "security measure" here so that we don't accidentally add a random member as admin. I'm wondering if there's a way to cross-check with like a CUReviews roster automatically.
| removePendingReview, | ||
| updateAllProfessorsDb, | ||
| resetAllProfessorsDb, | ||
| initAllDb, | ||
| addNewSemDb, | ||
| verifyTokenAdmin, | ||
| reportReview, | ||
| getAdminUsers, | ||
| removeAdmin, | ||
| addOrUpdateAdmin, | ||
| } from './admin.controller'; | ||
|
|
||
| export const adminRouter = express.Router(); |
There was a problem hiding this comment.
I think it would make it easier on the backend if all these routers were prepended by /admin. Like for getting reviews, that seems like it could belong to the display reviews page, but adding the admin route would help clarify that (and differentiate it from the user-facing reviews).
…ourse-reviews-react-2.0 into andrew/admin-fixes
There was a problem hiding this comment.
Awesome work Andrew!!! Glad we managed to get a lot of the major functionality of the Admin page working so far.
For other issues, we will move other issues to another PR through in the future though!
Note: discussed most issues in person and worked through updating code
|
[diff-counting] Significant lines: 1150. This diff might be too big! Developer leads are invited to review the code. |
Summary
This PR works on the Admin page feature, fixing old bugs and adding the ability to manage admin users by net-id.
PR Type
Mobile + Desktop Screenshots & Recordings
QA - Test Plan
Breaking Changes & Notes
Added to documentation?