feat: add content feedback model and submission endpoint - #3593
Conversation
OpenAPI Changes1 changes: 0 error, 0 warning, 1 info Unexpected changes? Ensure your branch is up-to-date with |
There was a problem hiding this comment.
Pull request overview
Adds a new self-contained Django app (content_feedback) that persists per-block learner feedback via a new authenticated DRF create endpoint, and updates the OpenAPI spec + generated TypeScript client to expose the new API contract.
Changes:
- Introduces
content_feedbackDjango app withContentFeedbackmodel, admin configuration, serializer, URL routing, and DRFCreateAPIView. - Adds API tests (including append-only resubmission and comment truncation behavior) plus a Factory Boy factory.
- Updates
openapi/specs/v0.yamland regeneratesfrontends/apiv0 client types/APIs for the new endpoint and sentiment enum.
Reviewed changes
Copilot reviewed 13 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| openapi/specs/v0.yaml | Adds /api/v0/content_feedback/ path and schemas (including SentimentEnum) to the v0 OpenAPI spec. |
| main/urls.py | Wires content_feedback URLs into the main Django URL config. |
| main/settings.py | Registers the new content_feedback app in INSTALLED_APPS. |
| frontends/api/src/generated/v0/api.ts | Regenerates the TS API client/types to include ContentFeedback endpoint + models. |
| content_feedback/views.py | Implements authenticated append-only submission endpoint (CreateAPIView). |
| content_feedback/views_test.py | Adds endpoint tests for auth, validation, append-only behavior, and truncation. |
| content_feedback/urls.py | Defines v0 URL routing for the new endpoint. |
| content_feedback/serializers.py | Defines ContentFeedbackSerializer including comment truncation logic. |
| content_feedback/models.py | Adds ContentFeedback model + indexes. |
| content_feedback/migrations/0001_initial.py | Creates initial DB schema for ContentFeedback. |
| content_feedback/factories.py | Adds ContentFeedbackFactory for tests. |
| content_feedback/constants.py | Defines sentiment enum and comment max length constant. |
| content_feedback/apps.py | Declares app config. |
| content_feedback/admin.py | Adds read-only Django admin for viewing submissions. |
| content_feedback/init.py | Initializes package. |
| content_feedback/migrations/init.py | Initializes migrations package. |
a7e0963 to
f495fd2
Compare
|
Good catch — addressed in 254aa8c: dropped |
aa7c61c to
254aa8c
Compare
| from main.models import TimestampedModel | ||
|
|
||
|
|
||
| class ContentFeedback(TimestampedModel): |
There was a problem hiding this comment.
Future extension: generalizing to non-course ("feedback on mit-learn itself", Appzi-style)
This app is intentionally scoped to per-block course content feedback. If/when we need to collect feedback on mit-learn pages themselves (search, resource detail, features), the plan is to extract the shared fields into an abstract base and add a sibling concrete model — not to overload ContentFeedback:
BaseFeedback (abstract) # user, comment, url, created_on, updated_on
├── ContentFeedback # + course_id, block_usage_key, block_type, …, sentiment
└── SiteFeedback (new) # + page_type/route, resource_id (or GenericFK), rating/nps
Each domain keeps its own flat, queryable columns (good for the data platform); shared logic lives once in the base.
Why later, not now:
-
No second consumer yet. The right shared shape is only knowable once site-feedback requirements are real (e.g. does it use sentiment or an NPS/star rating? key off url or a resource id?). Abstracting today means guessing — and a wrong abstraction costs more than none.
-
Retrofitting is near-free. An abstract base creates no table of its own, so extracting it later is a no-op/state-only migration — existing ContentFeedback rows are untouched, and SiteFeedback is a purely additive new table.
-
Keeps this PR focused. Speculative inheritance adds review surface for an unscoped use case with zero functional gain today.
254aa8c to
160c7d5
Compare
New content_feedback app: ContentFeedback model (append-only, user set server-side, on_delete=SET_NULL), POST /api/v0/content_feedback/ via CreateAPIView with enum/required-field validation and comment truncation, read-only admin, single 0001 migration, factory, and 11 tests. Registers the app in settings/urls and regenerates the OpenAPI spec + v0 TS client.
160c7d5 to
ac1e008
Compare
mbertrand
left a comment
There was a problem hiding this comment.
LGTM, I think it would be good to change the url field from a TextField to a URLField or CharField, the other comments are just minor nitpicks.
| block_type = models.CharField(max_length=64, blank=True) | ||
| block_display_name = models.CharField(max_length=255, blank=True) | ||
| unit_title = models.CharField(max_length=255, blank=True) | ||
| url = models.TextField(blank=True) |
There was a problem hiding this comment.
Consider URLField(max_length=2083, blank=True) here — bounds the column (TextField is unbounded, so any authed user can store arbitrarily large values) and validates format at the serializer layer for free. 2083 matches the URL-ish fields elsewhere in the repo. Cheap to do now while 0001_initial hasn't shipped.
| def has_add_permission(self, request): # noqa: ARG002 | ||
| """Disallow creating records in the admin; they come only via the API.""" | ||
| return False | ||
|
|
||
| def has_delete_permission(self, request, obj=None): # noqa: ARG002 |
There was a problem hiding this comment.
nit: add/delete are disabled but the change form still saves — with everything in readonly_fields it's a no-op edit, except it bumps updated_on on an append-only record. Adding has_change_permission → False closes that; records stay browsable via the view permission.
| block_display_name = models.CharField(max_length=255, blank=True) | ||
| unit_title = models.CharField(max_length=255, blank=True) | ||
| url = models.TextField(blank=True) | ||
| sentiment = models.CharField( |
There was a problem hiding this comment.
nit: drf-spectacular names the spec component SentimentEnum, which is pretty generic for a global name — if another v0 serializer ever adds a sentiment field with different choices, it'll warn and auto-suffix. An ENUM_NAME_OVERRIDES entry (or naming it ContentFeedbackSentimentEnum) now is cheaper than renaming after the MFE imports the type.
476c81f to
27144c1
Compare
|
@mbertrand Could you please re-review when you get a chance, I've addressed the feedback. |
* feat: add content_feedback backend (append-only model + submit endpoint)
What are the relevant tickets?
Part of the per-block content feedback effort — RFC mitodl/hq#11812 (parent issue mitodl/hq#11629).
Companion PRs:
mitodl/smoot-design#241— feedback drawer UI bundlemitodl/open-edx-plugins#813— in-iframe "Send feedback" trigger (ol_openedx_feedback)mitodl/lehrer#83— Learning MFE slot wiringSupersedes the earlier learn-ai backend PR
mitodl/learn-ai#556(to be closed).Description (What does it do?)
Adds the mit-learn backend that persists per-block content feedback submitted from the Learning MFE. New self-contained
content_feedbackapp, following the conventions of small single-purpose apps liketestimonials.ContentFeedbackmodel —user(FK, nullable,on_delete=SET_NULLso feedback outlives a deleted account),course_id,course_name,block_usage_key,block_type,block_display_name,unit_title,url,sentiment(positive/negative/idea),comment. Composite index on(course_id, created_on);block_usage_keyindexed.created_on), not enforced in the schema.POST /api/v0/content_feedback/— a DRFCreateAPIView(IsAuthenticated);perform_createattributes the record to the authenticated user server-side (useris never client-settable). Returns 201 with the created record, or 400 on validation errors. Auth is the same APISIXx-userinfo→ session mechanism used across mit-learn.ContentFeedbackSerializer—sentimentvalidated against theContentFeedbackSentimentenum;course_id/block_usage_key/sentimentrequired;commenttruncated to 1000 chars (not rejected).created_on), a factory, a single0001_initialmigration, and 11 view/serializer tests.Screenshots:
How can this be tested?
1. Automated (standalone — no gateway/auth needed):
All 11 tests pass. They cover: auth required, record created & attributed to the request user,
usernot client-settable, required-field validation, invalid sentiment rejected, append-only resubmit keeps every record (2 rows, history preserved), distinct blocks kept separate, comment truncation, factory validity.2. Live endpoint (through APISIX, real auth path):
The endpoint is
IsAuthenticated+SessionAuthentication+ CSRF, with the user established by APISIX's injectedX-Userinfo. It therefore can't becurl'd cold — you need a logged-in session and a CSRF token. The browser console is easiest (it carries the session cookie automatically):docker compose up -d, then log in: openhttp://open.odl.local:8065/loginand complete Keycloak. (Hitting/api/...won't prompt — only/logintriggers the OIDC login. Sessions lapse, so if you getNotAuthenticated, log in again.)open.odl.local:8065origin, open the devtools console and run the snippet below. Note the local CSRF cookie iscsrftoken-local(deployment-prefixed, env-configurable — not the barecsrftoken); the header isX-CSRFToken.GET /api/v0/users/me/is@ensure_csrf_cookie, so it primes the cookie. The body below sends every accepted field (course_id,block_usage_key,sentimentare required; the rest are optional context the MFE enriches):Expect 201 with the validated record and a new
ContentFeedbackrow attributed to the authenticated user. POSTing again for the same block creates an additional row (append-only). This is the same mechanism the MFE drawer uses (credentials + CSRF token echoed from the cookie).This data can be viewed at http://open.odl.local:8065/admin/content_feedback/
Additional Context
Open items before go-live (integration, handled outside this PR):
GET /api/v0/users/me/(@ensure_csrf_cookie) rather than the edX-style/csrf/api/v1/tokenprefetch; the client wiring needs that swap.CORS_ALLOWED_ORIGINSandCSRF_TRUSTED_ORIGINS(both env-driven — no code change here).FEEDBACK_SUBMIT_URLpoints at this endpoint once it lands.learning_resourcesFK intentionally omitted for v1 (a leafblock_usage_keyhas no guaranteed catalog row); the model can add a nullable FK later.