Michelle's Winter Break Work

[michelleli01]

Summary

This is my final server-side refactoring to ensure more modularity within each component on the server side making it easier for future refactoring, incorporating new functionality, and the entire development experience as a whole. I've also fixed all admin button functionality on the client and server side.

Completed Tasks

• improve search functionality (fix up regex search, edit distance algo, etc)
• modularize routes with traditional express routers
• utilize response status codes instead of response objects
• separate architecture with controller layer (business logic), data access layer (crud functions in mongo), and types (request types vs internal server types)
• alter verification of client requests through the use of internal entities validated using joi to ensure type safety (required field, regex expressions, etc)
• fix report button functionality (added it back as well)
• separate course api scripts (updating professors, resetting professors, adding new semester, initializing db)
• fix all admin action functionality (adding new semester, initializing db, resetting professors, updating professors)
• added functionality back to client side admin page
• make server side logic more type safe
• clean up jest tests (abstracting repeated functionality with mocks)
• more robust unit testing
• started some e2e tests with cypress for search functionality
• update course metrics when necessary (review reported, review approved, etc.)
• fix/clean up jest unit testing
• make functions more type safe
• figure out cross listed course logic
• document all changes and detail where all functionality is (router, controller, data-access, etc)
• document code in codebase
• added css variables in line with the design system as of sp22 for consistent colors and font sizing (we had like 10 different grays)
• cleaned up some css on the home page and results page

Test Plan

Admin Page with working buttons:

Tested locally on local server with jest unit tests and cypress e2e tests.

Leaving a review:
https://github.com/cornell-dti/course-reviews-react-2.0/assets/11303631/36fad35b-a8c7-4f16-b001-3a6ac6740376

Reporting review and admin deleting reported review:
https://github.com/cornell-dti/course-reviews-react-2.0/assets/11303631/d110cd2c-213c-43e0-a006-7b0ba4e64fe4

Admin approving review and profile page display:
https://github.com/cornell-dti/course-reviews-react-2.0/assets/11303631/97527f64-8f3f-40cd-aed8-ba8bd26bbad2

Notes

There are some new dependencies added (joi and cypress) so make sure to yarn install again.

joi is used to validate the various objects that represent entities in the server side logic (ex. ensuring search queries are string, tokens are required, etc).

cypress is used for end to end testing, which I've kicked started in writing some tests. CU Review is big enough and has enough e2e functionality where I think automating this kind of testing will be beneficial. Of course, focus on unit tests they can definitely be more comprehensive and are infinitely more efficient to write and catch bugs.

The fixed admin functionality is a bit shoddy at best so client side messages aren't always accurate sometimes (async isn't blocking). But, all buttons work properly and will update the database appropriately with proper classes and professors (just be very patient some of these tasks are VERY long running).

I did not fix the admin chart functionality since I have never seen that functionality work ever and got very tired of staring and trying to piece together our messy server code.

Future Work (for consideration)

• There are a lot of unnecessary API calls occurring repeatedly from the client side. While this doesn't reduce performance, it's not the most robust way of doing things and can cause some confusion when debugging problems.
• Consider refactoring authentication (the way we currently set up google oauth is definitely not standard and it's quite painful since we can't get all necessary user information like the given_name or family_name... not necessary info though)
• Database models are just a mess.... there's redundancy everywhere. Consider using an orm like prisma or drizzle to help manage all this (an orm also adds a layer of abstraction, which may be helpful).
• Testing testing testing I found a lot of wack bugs while refactoring everything, which really can be prevented with more testing. It's annoying, but useful in the long run.
• Also consider getting rid of class components... that is very old school.

FINAL NOTE

The CSS for CU Reviews is some of the most atrocious code I have personally ever seen . That should be fixed. It's very painful to work with CSS on CU Reviews 👎 I've tried to fix some stuff up, but I am quite inadequate at CSS.

Exhibit A:

Exhibit B:

Exhibit C:

[wizhaaa]

Hi Michelle - thanks so much for the hard work over the break. This backend restructuring is extremely beneficial in making the codebase more usable, readable, and adaptable for our future features.

I sent a few messages over Slack with some issues and clarifications on how the infrastructure of the application works. I'll put them in this comment.

• So, Q: local development we run yarn start for client (port 3000) & server (port 8080). Frontend calls API through axios each time, not an instance of axios with a baseURL (ie. 8080 || cureviews/api). This works because we specify a proxy to 8080.
• For production, this is irrelevant because the server itself serves the built react files in the build/ folder.

However, besides these few issues - technically it should be fine - as my main concerns existed before this PR was made. Other than this small issue, I think this PR is good to go.

[wizhaaa]

Also - I 100% agree with ur CSS comments. I mentioned this earlier as well. I suspect that a root cause of the styling being different on the local environments and production environments may also be due to Bootstrap. I saw your edits to the profile page which removed bootstraps row and col classes which is nice!

I tell the devs to also remove any row or col they see when they work on a frontend change.

But we still have to completely remove the bootstrap dependency which may or may not break every page's and modals styling => so not 100% if it may be better to rebuild everything. Currently, I'm thinking of rebuilding a page first and then replacing it once it works instead of editing current pages.

Or, as I try to run master, there are a lot of dependency issues now and outdated packages like TS, React, Router, ESLint, etc. I feel it may be better to rebuild the entire repo into a new one.

[michelleli01]

Hi Michelle - thanks so much for the hard work over the break. This backend restructuring is extremely beneficial in making the codebase more usable, readable, and adaptable for our future features.

I sent a few messages over Slack with some issues and clarifications on how the infrastructure of the application works. I'll put them in this comment.

• So, Q: local development we run yarn start for client (port 3000) & server (port 8080). Frontend calls API through axios each time, not an instance of axios with a baseURL (ie. 8080 || cureviews/api). This works because we specify a proxy to 8080.
• For production, this is irrelevant because the server itself serves the built react files in the build/ folder.

However, besides these few issues - technically it should be fine - as my main concerns existed before this PR was made. Other than this small issue, I think this PR is good to go.

Thanks for the quick and comprehensive review Will! I know that this PR is quite large... I addressed your questions on Slack. But I'll TLDR my response here:

The proxy in the client/package.json just allows us to redirect any client requests from port 3000 to port 8080 while still allowing the client and server to run on their respective ports. So, the client runs on port 3000 and when it makes a request /api/... that will be redirected to port 8080 where the server can handle all the operation and return a response. In production though, this proxy is totally ignored and so the server basically handles all the serving for any CU Reviews static files AND any api requests.

[michelleli01]

Also - I 100% agree with ur CSS comments. I mentioned this earlier as well. I suspect that a root cause of the styling being different on the local environments and production environments may also be due to Bootstrap. I saw your edits to the profile page which removed bootstraps row and col classes which is nice!

I tell the devs to also remove any row or col they see when they work on a frontend change.

But we still have to completely remove the bootstrap dependency which may or may not break every page's and modals styling => so not 100% if it may be better to rebuild everything. Currently, I'm thinking of rebuilding a page first and then replacing it once it works instead of editing current pages.

Or, as I try to run master, there are a lot of dependency issues now and outdated packages like TS, React, Router, ESLint, etc. I feel it may be better to rebuild the entire repo into a new one.

Hmm you're right. Bootstrap is probably the problem good catch! We should maybe consider migrating off Bootstrap? I'm personally a big fan of Tailwind lol, but I feel like plain CSS modules will be just fine and cause less of a headache. A lot of the problems with the CSS I was seeing were being caused by side effects from the row and col classes.

Q: Should I remove my CSS changes, so you can handle any frontend migrations you want to do?

Reconfiguring the repo might be nice since I know there are a lot of outdated dependencies. But, that is a bit more painful. I would probably

1. Rebuild the pages and and migrate off Bootstrap and maybe introduce a new framework?? (i like tailwind lol, but moving to just using css modules + css variables + utility classes will be just fine)
2. Upgrade outdated dependences

But, that is totally up to you!

[wizhaaa]

Got it - I understand now, thanks so much!

One thing I was thinking of doing this semester was changing the routes to be RESTful such as:

• Route names to be instead of getX or removeX to app.get(x/:id) or app.delete(x/:id)

When I was testing, I tried some routes on Postman like app.get or adminRouter.get which kept getting errors, thus I was confused on why they were not working. But, I see that it is because all GET requests serve the frontend (build/). So, for my plan on RESTful, unless we migrate to two scripts in prod, only thing can do is change the route name maybe but not the request type.

But, this is not super important. This was just to clarify on these routes that were not working and realize, they actually do work. And, the only one that was really a 404 was the admin updating the new semester which was just a typo you already fixed.

---

Q: Should I remove my CSS changes, so you can handle any frontend migrations you want to do?

For the CSS, I think you can keep your changes. I also really like tailwind (clean + simple) but I think it is more advantageous to use our own custom CSS so that we can more accurately replicate designs unless our desingers also migrate our design systems to tailwind.

---

Reconfiguring the repo might be nice since I know there are a lot of outdated dependencies. But, that is a bit more painful. I would probably
Rebuild the pages and and migrate off Bootstrap and maybe introduce a new framework?? (i like tailwind lol, but moving to just using css modules + css variables + utility classes will be just fine)
Upgrade outdated dependences

Yup - I agree, this is the ideal way. I'll try my best - is kind of rough like last semester's react-router upgrading was not cooperating well and was tough to debug.

---

Else, I will approve of this PR. I think the functionality is there, the code is restructured well and this is a great starting point to work off of and add more changes to.

Once this PR is merged, I'm thinking of adding some comments/explanations in the backend & notion documents and some other minor frontend changes (maybe try to remove all bootstrap too) for the start of the semester.

[wizhaaa]

Approving - main comments addressed & great functionable restructuring to work off of. Thank you so much for the hard work!

[michelleli01]

One thing I was thinking of doing this semester was changing the routes to be RESTful such as:

• Route names to be instead of getX or removeX to app.get(x/:id) or app.delete(x/:id)

When I was testing, I tried some routes on Postman like app.get or adminRouter.get which kept getting errors, thus I was confused on why they were not working. But, I see that it is because all GET requests serve the frontend (build/). So, for my plan on RESTful, unless we migrate to two scripts in prod, only thing can do is change the route name maybe but not the request type.

But, this is not super important. This was just to clarify on these routes that were not working and realize, they actually do work. And, the only one that was really a 404 was the admin updating the new semester which was just a typo you already fixed.

This is a good idea! I was thinking about converting all the endpoints to RESTful too, but I got lazy 😆. One thing that would probably work is just changing the app.get(*, ...) to just app.get('/', [serve index.html] and then proceeding with converting the necessary endpoints from .get to post. We can just get rid of the catch all since we have a 404 Not Found page.

Q: Should I remove my CSS changes, so you can handle any frontend migrations you want to do?

For the CSS, I think you can keep your changes. I also really like tailwind (clean + simple) but I think it is more advantageous to use our own custom CSS so that we can more accurately replicate designs unless our desingers also migrate our design systems to tailwind.

Sounds good!

Else, I will approve of this PR. I think the functionality is there, the code is restructured well and this is a great starting point to work off of and add more changes to.

Once this PR is merged, I'm thinking of adding some comments/explanations in the backend & notion documents and some other minor frontend changes (maybe try to remove all bootstrap too) for the start of the semester.

Oof that's a lot of work. I can add some comments to the backend code before merging. I also detailed some thoughts in this Notion page. But, if you need me to explain my thought process further I would be happy to do so.

Thanks for all the hard work Will! I know working on such an large, old product is very frustrating at times since it requires such large scale refactors like this one if previous TPMs aren't careful (my bad lol). But, you're doing a great job!! Let me know if you need any help upgrading infrastructure and I'll see what I can do.

Excited to see CU Reviews do big things next sem 😄