Conversation
|
[diff-counting] Significant lines: 2055. This diff might be too big! Developer leads are invited to review the code. |
|
This is a great step towards the finished blog feature! The api routes are really organized and easy to read—the documentation is especially great and will be helpful to reference later on. Can't wait to see the finished feature! |
CasperL1218
left a comment
CasperL1218
left a comment
There was a problem hiding this comment.
-
For this
new-blog-postendpoint, the current implementation is using the 401 error code also for handling missing content fields, which is not covered in the documentation. This should be handled either with a different error code or you should adjust the endpoint spec to reflect this. Here is the current endpoint specification. -
For the
delete-blog-postendpoint, the comments mentioned that it will be checking for either admin access or blog post creator access. However, the current code is only conducting admin check, which to be honest should be enough considering our current plan of use for this feature. Nevertheless, it would be a good idea to consider adding a creator field to specify the ownership of the blog post and conduct corresponding authentication checks. -
For the
edit-blog-postendpoint, I believe you have commented out the authentication portions (possibly for testings). Remember to uncomment them later. -
The
userIdfield in the type definition for the blog post need to be clearer. I assume it should be referring to the creator of the blog post, so you should rename it to be more specific.
Overall, the structure of the endpoints are well-organized. The update for the handle-like endpoint for different targets are well done. Keep up with the detailed documentations and be careful about consistency between them and the actual implementation. Good job!
laurenp-2
left a comment
laurenp-2
left a comment
There was a problem hiding this comment.
This is overall a great foundation for the BlogPosts! The most important changes are to make sure that your endpoints match the specifications and that all behavior is documented (so making sure that the http status codes are reflected in the specification and also that the archive vs delete behavior is explained clearly in documentation). Overall super excited to see the full feature!
| !blogPost.tags || | ||
| !blogPost.visibility | ||
| ) { | ||
| res.status(401).send('Error: missing fields'); |
There was a problem hiding this comment.
Although an error is raised, since its not returned the code will continue and create a doc anyway (also, in the spec you say 401 is due to authentication issues—400 Bad Request might fit better)
| * - 401: Error due to unauthorized access or authentication issues. | ||
| */ | ||
| app.post('/api/edit-blog-post/:blogPostId', async (req, res) => { | ||
| // if (!req.user) { |
There was a problem hiding this comment.
if testing is complete don't forget to uncomment this!
backend/src/app.ts
Outdated
| } | ||
| try { | ||
| // Update the status of the blog post document to 'DELETED' | ||
| await blogPostCollection.doc(blogPostId).update({ status: 'ARCHIVED' }); |
There was a problem hiding this comment.
There seems to be a mismatch between archiving and deleting blog posts. It might be helpful to describe the difference in the specifications!
- Updated engines.node from "16.x" to ">=18"
- Added resolutions to pin React types: @types/react: 17.0.80, @types/react-dom: 17.0.25
2. frontend/package.json:
- Updated react-scripts from 4.0.0 to ^5.0.1
- Updated typescript from ~4.0.5 to ^5.0.0
- Updated @types/react from ^16.9.53 to 17.0.80
- Updated @types/react-dom from ^16.9.8 to 17.0.25
- Added eslint-config-react-app
3. backend/package.json:
- Updated typescript from ^4.0.5 to ^5.0.0
4. frontend/src/utils/firebase.ts:
- Removed unnecessary dotenv import and config() call
5. frontend/src/components/Apartment/MapInfo.tsx:
- Removed unnecessary dotenv import and config() call
6. frontend/src/components/Search/SearchResultsMap.tsx:
- Removed unnecessary dotenv import and config() call
7. frontend/src/react-app-env.d.ts:
- Added global firebase.User type declaration for TypeScript compatibility
8. yarn.lock:
- Updated with new dependency versions
- .github/workflows/ci.yml - Node 16 → 18 - .github/workflows/firebase-hosting-merge.yml - Added Node 18 setup step - .github/workflows/firebase-hosting-pull-request.yml - Node 16 → 18
…on, and continued work on blog post frontend
…ixes and adding templates, also created blog post detail pages where content can be viewed and made minor tweaks to some routes
4 participants
Summary
This PR focuses on both the frontend and the backend for my blog post feature. The feature is still a work in progress and will be completed sometime by the end of the semester.
So far, most of the frontend has been completed, with only one task left to complete. For the frontend on the admin side, which would allow for the team to actually create and post blogs, all that's left is to polish parts of the blog post creation feature and start on the blog post activation portal for archiving or posting blogs
The backend is now complete. Every route has been created, documented, and tested, and some older routes were tweaked in conjunction with the change in our database schema
Test Plan
To test the routes, I used Postman for each route and checked them based on the changes made in the database. I turned of authentication for each route to simplify the process. Here is a picture of all the routes I have tested so far on Postman
And here are some preliminary screenshots of the current state of the frontend