feat(courses): let an educator edit a course they have published - #132
Open
MOHITKOURAV01 wants to merge 2 commits into
Open
feat(courses): let an educator edit a course they have published#132MOHITKOURAV01 wants to merge 2 commits into
MOHITKOURAV01 wants to merge 2 commits into
Conversation
grep -rn "router.put\|router.patch" backend/routers returns one line, and it is a review. A course could be created and deleted and nothing else, so correcting a typo in a title, or a price entered as 499 when it should have been 4990, meant deleting the course and building it again. Deleting is destructive by design (udaycodespace#74): removeCourseDependents takes every enrolment — and with it every student's progress — every payment, review and bookmark, and every section video on disk. Fixing one character cost all of that plus the whole upload, and for a paid course every enrolled student paying again. None of the fields that actually need correcting touches a file or invalidates an enrolment. - GET /api/user/editcourse/:courseid returns the course as a form needs it. It exists because udaycodespace#94 deliberately projected section text out of the educator list response, so a form that renames a section has nowhere else to read the current name. S_content is not in it. - PUT /api/user/editcourse/:courseid applies the change. JSON, not multipart, so there is no Multer and none of the identity trouble from udaycodespace#83. Both are on the admin router too, the way deletecourse already is. - An allow-list, not a spread — {...req.body} is how udaycodespace#55 happened. Four metadata fields and two per-section text fields; userId, enrolled, _id and a section's stored file path are ignored, with a test that sends all four. - The section count is fixed by the uploads this route cannot change, so a list of a different length is refused rather than silently truncated. - C_educator is re-read from the token on the owner's edit. It is written once at creation (udaycodespace#83), so a teacher who later corrected their name left the old byline on everything already published. An admin editing someone else's course does not become its educator. - An Edit action on each card, filled from the GET, with Save disabled while the form is untouched so "No editable fields were supplied" is never a sentence anybody reads. Ownership is decided the way deletion decides it: a teacher may edit their own course, an admin any course. backend 531 pass (505 before), frontend 232 pass (214 before). Closes udaycodespace#127
Both were being inserted at the same anchor as the playback token refresh in udaycodespace#124, so the two produced a conflict on a pair of independent additions. Beside courseDeletionController is where this belongs anyway: the two routes share their ownership rule, and editing exists so that correcting a title no longer has to go through deleting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #127.
The defect
A course could be created and deleted. It could not be changed.
One
PUTin the project, and it belonged to a review.userRoutes.jsofferedPOST /addcourseandDELETE /deletecourse/:courseid.TeacherHome.jsxrendered one action per card,
Delete.AddCourse.jsxalways posted to/addcourse— no edit mode, and no component to put one in.Why "delete and re-add" is not a workaround
deleteCourseControlleris deliberately destructive, and correctly so — #74made it that way:
So fixing one character in
C_titlecost every enrolment and with it everystudent's
progress; every payment record, which is the admin ledger's onlyhistory of who paid for what; every review and the catalogue card's rating;
every bookmark; and every
.mp4, which then had to be uploaded again. For apaid course it also meant every enrolled student paying again.
None of the fields an educator actually needs to correct touches a file on disk,
and none of them invalidates an enrolment.
What is here
GET /api/user/editcourse/:courseid— the course as an edit form needs it.It exists because #94 deliberately projected
sectionsout of the educator'slist response along with the stored file paths, so a form that renames a section
has nowhere else to read the current name from. The response carries section
titles, descriptions and a
hasVideoflag, and notS_content: an editform has no more reason to see a storage path than a catalogue card does.
PUT /api/user/editcourse/:courseid— the change. JSON, not multipart: noneof the editable fields is a file, so there is no Multer here and none of the
identity trouble that comes with it (#83). Both are mounted on the admin router
too, the way
deletecoursealready is.An allow-list, not a spread.
{ ...req.body }is how #55 happened. Fourmetadata fields and two per-section text fields are read; everything else is
ignored rather than rejected, so a form that posts the whole course document
back still works and still cannot write
userId,enrolled,_idor asection's
S_content. There is a test that sends all four, and another thatsends a section
S_contentof{ path: "/etc/passwd" }and asserts the storedpath is untouched.
The section count is fixed. This route takes no uploads, so a submitted list
of a different length is refused with a sentence saying so, rather than
truncating or padding silently.
A stale byline is fixed on the way through.
C_educatoris written once atcreation from the token (#83), so a teacher who later corrected their name left
the old one on every course they had already published. An edit by the owner
re-reads it from the token; an admin editing somebody else's course is not the
educator, so their name is not written — asserted by a test.
Ownership is decided the way deletion decides it: a teacher may act on a
course they own, an admin on any course.
An
Editaction on each educator card, opening a modal filled from the GET.Save is disabled while the form is untouched, so "No editable fields were
supplied" is never a sentence anybody has to read. The price field states the
#114 rule out loud — leave blank, or enter 0 or "free", for a free course —
rather than leaving it to be discovered by submitting.
What an edit deliberately cannot do
size and count rules ([Security]: Strengthen MP4 upload validation and enforce file-size limits #44, [Bug]: Add Course offers file types the API rejects, and a 250 MB upload only fails once it has finished uploading #106) and a cleanup path for a rejected one.
userId. Ownership comes from the token at creation and stays there.enrolled. It is a count of real enrolments, not a field.Tests
backend/tests/course-edit.test.js— 26 tests with injected models, so nodatabase. A teacher editing somebody else's course is refused and the stub
records no write; a student is refused by a controller whose
Course.findByIdthrows, so the test fails if the lookup is ever reached; and after a successful
edit the stub still holds
enrolled: 42, the originaluserIdand bothsections — everything the delete-and-recreate workaround would have destroyed.
frontend/src/lib/courseEdit.test.js— 18 tests, including that whitespacealone is not a change and that a course whose
sectionsfield is an object mapproduces an empty list rather than throwing, which is the shape that blanked the
whole educator dashboard in #94.
docs/issue-127-edit-course.mdhas the write-up.Checklist
cd backend && npm test— 531 pass (505 onmain, 26 added)cd frontend && npm test— 232 pass (214 onmain, 18 added)cd frontend && npm run buildnpm run linton the touched frontend files —lib/courseEdit.js,EditCourse.jsxandTeacherHome.jsxare all clean. The repo-wide runstill reports the 69 pre-existing problems it reports on
main.