Allow admins to reject resource suggestions #38
Conversation
There was a problem hiding this comment.
Generally, this is very impressive: well done! 👏
As an iteration, I'd like to see:
- 🧪 At least one additional test at a different level (i.e. E2E or API integration, not just the frontend React tests)
- 📚 Document the API endpoint (either as-is, or you can update as suggested and document the new version)
- ❓ How would you meet the implied requirement of soft deletion (you don't have to implement it, but I'd be interested to hear how you'd approach it)?
(If you'd like to practice on implementing any of the other feedback then you're welcome to, but don't feel you have to.)
Functionality
If I Suggest a draft (as admin or regular user), then visit the Drafts page as an admin, I see:
-
✅ If I click the "Reject" button, the resource is removed from the Drafts page
-
✅ The resource doesn't appear on the home page either
-
❌ The resource is hard-deleted from the DB, note the story links:
- Scenario 2 in Show users their own resource suggestions #15 implies soft deletion
-
❌ UI/UX of deletion could be improved:
- Action buttons feel weirdly far apart, it might make more sense to have the buttons in a shared container in the bottom right hand corner
- Action buttons are the same colour, easy to mix up at a glance
- It's a good principle of interaction design (especially given point 2 and the hard delete) to require confirmation (even a basic
window.confirmwould do) before doing things that could lead to data loss
Implementation
-
✅ Generally very good; you've put things in sensible places and everything works as you intended
-
❌ From an API design perspective, a
DELETErequest would generally:- Have no request body (the resource ID is already in the path)
- Have no response body (the resource is now "gone" so there's nothing to return)
- Have a status of 204 No Content
-
❌ No E2E test of the new functionality
-
❌ No integration test or documentation of the new API endpoint, which has a few cases to cover (401 Unauthorized, 403 Forbidden, 404 Not Found and the successful delete -> 200 OK (as-is)/204 No Content (to-be))
Key
- ✅ Nothing to change, just commenting
- 💡 Possible change, or suggestion to think about in the future
- ❓ Discussion required before approval
- ❌ Change required before approval
| validated({ | ||
| body: Joi.object({ | ||
| draft: Joi.any().valid(false), | ||
| }), | ||
| }), |
There was a problem hiding this comment.
❌ In line with the feedback about API design there shouldn't be any required body/validation
| }), | ||
| asyncHandler(async (req, res) => { | ||
| try { | ||
| res.send(await service.reject(req.params.id)); |
There was a problem hiding this comment.
❌ And rather than 200 OK with a body, 204 No Content is more idiomatic
| if (err instanceof service.MissingResource) { | ||
| logger.info(err.message); | ||
| return res.sendStatus(404); | ||
| } |
There was a problem hiding this comment.
✅ Nice to see the error handling pattern being used, and 404 is an appropriate response status code here 👍
❌ If it's not a MissingResource, the error has now been caught but the request has still not been responded to, you need to:
| if (err instanceof service.MissingResource) { | |
| logger.info(err.message); | |
| return res.sendStatus(404); | |
| } | |
| if (err instanceof service.MissingResource) { | |
| logger.info(err.message); | |
| return res.sendStatus(404); | |
| } | |
| throw err; |
so it reaches the global error handler and becomes a 500 Internal Server Error or the request will hang indefinitely.
Hi @textbook, Can I change the schema of resources? As I understood form the other issue, when the admin reject a resource, user should still see it in another tab to track all own suggested resources. In resources table, we have a draft column which is true by default, then after publish, it would change to false. So how should I flag it as a rejected? Wasn't better if we had a status column (draft / published / rejected)? |
The migrations are there to allow us to evolve the schema as needed.
Yes, that would be a way to do it - with a boolean we can only represent two states, "draft" or "not draft" (approved). Moving to something with more than two possible values (maybe an enum or look it up from another table, see e.g. https://stackoverflow.com/q/24680744/3001761 for trade offs) allows representing a wider range of states, including "rejected".
Currently there's no editing supported and none of the issues propose it, so I wouldn't spend time planning for that now. |
farzaneh-haghani
force-pushed
the
feature/reject
branch
2 times, most recently
from
5eb8f5e
to
f5cd727
Compare
farzaneh-haghani
force-pushed
the
feature/reject
branch
4 times, most recently
from
a76c3d9
to
b5caa96
Compare
farzaneh-haghani
force-pushed
the
feature/reject
branch
from
b5caa96
to
549ff1c
Compare
|
@textbook Could you please review my changes again. |
farzaneh-haghani
force-pushed
the
feature/reject
branch
from
b282841
to
9402383
Compare
|
I haven't reviewed it in as much detail this time, but overall that's a good iteration, thank you!
✅ Added E2E test "lets admin users reject drafts"
✅ Existing docs were updated (DELETE endpoint was removed, so no docs needed) ❌ OpenAPI lets you document enums directly, not just as
✅ Implemented by switching from boolean to enum status, so rejection sets resources to ❌ As in the docs string is too broad for the status parameter, Joi also lets you represent enums https://joi.dev/api/?v=17.13.0#anyvalidvalues---aliases-equal |
|
@textbook Thank you. Why did you close it? I can improve it to be merged? |
|
It's just an exercise, a work simulation. If we merged all of the applications I'd have to keep coming up with new stories (and keep the PR review task branch up-to-date with the changes)! |
closes #13
This is a:
Description
It will show a reject button to allow admin to reject a resource in the draft. It will delete the resource from db.
Allow admin to reject a suggested resource.
Log in as astronaut, go to suggest tab and fill the from. Then log out and log in as admin and go to draft and find your suggestion. Previously did not have a reject button but now it have and work.
Links
Author checklist
mainfrom a branch named<category>/<name>, e.g.feature/edit-spaceshipsorbugfix/restore-oxygen