Epic 0: scope lock + core documentation#82
Conversation
There was a problem hiding this comment.
Pull request overview
This PR establishes the complete foundational documentation for CorvEd, a managed O/A Levels tutoring platform. The PR successfully closes Epic 0 by defining the MVP scope, product vision, operational playbook, technical architecture, and development roadmap. The documentation is comprehensive and provides clear guidance for implementation.
Changes:
- Added complete MVP documentation suite covering product vision, policies, operational workflows, technical architecture, and phased roadmap
- Created README.md with local development setup instructions and repo structure overview
- Added GitHub issue templates (bug, story, task) and PR template with structured forms
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/ROADMAP.md | Defines phased delivery strategy from concierge validation through MVP v0.1/v0.2 with sprint plans and risk register |
| docs/PRODUCT.md | Specifies product vision, target users, service offerings, UX requirements, pricing model, and success metrics |
| docs/OPS.md | Provides operational playbook with WhatsApp workflows, message templates, policy enforcement procedures, and admin checklists |
| docs/MVP.md | Locks MVP scope including policies (24hr reschedule, no-show rules), package tiers, user roles, data model, and definition of done |
| docs/ARCHITECTURE.md | Details technical implementation with Next.js + Supabase, complete DB schema, RLS policies, RPC functions, and workflow sequences |
| README.md | Documents tech stack, repo structure, local development setup with Supabase CLI, and contribution workflow |
| .github/pull_request_template.md | Provides structured PR template with summary, issues, changes, testing checklist, and notes sections |
| .github/ISSUE_TEMPLATE/bug.yml | Comprehensive bug report form with priority, component, reproduction steps, environment, and reporter checklist |
| .github/ISSUE_TEMPLATE/story.yml | User story template with acceptance criteria, dependencies, designs, and implementation notes |
| .github/ISSUE_TEMPLATE/task.yml | Engineering task form with objectives, steps, definition of done, dependencies, risks, and validation plan |
| .github/ISSUE_TEMPLATE/config.yml | Issue template configuration with contact links for WhatsApp support and ops playbook (contains placeholders) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| url: https://wa.me/<YOUR_NUMBER_IN_E164> | ||
| about: For student/parent support requests (non-technical). | ||
| - name: CorvEd Ops Playbook | ||
| url: https://github.com/<YOUR_ORG>/<YOUR_REPO>/blob/main/docs/OPS.md |
There was a problem hiding this comment.
The URL "https://github.com/<YOUR_ORG>/<YOUR_REPO>/blob/main/docs/OPS.md" contains placeholder values for organization and repository. This should be updated with the actual GitHub organization and repository names before merging or launching.
| url: https://github.com/<YOUR_ORG>/<YOUR_REPO>/blob/main/docs/OPS.md | |
| url: https://github.com/corved-org/corved-repo/blob/main/docs/OPS.md |
| - no carryover of unused sessions into next month | ||
|
|
||
| 5.3 reschedule policy | ||
| - reschedule cutoff: 24 hours before the scheduled session start time (based on the student’s timezone for clarity; store rule enforcement in UTC) |
There was a problem hiding this comment.
Issue #2 recommends a "reschedule cutoff (recommend 12 hours)" but the documentation consistently uses 24 hours throughout (MVP.md line 155, PRODUCT.md line 248, OPS.md line 309). There's a discrepancy between the issue recommendation and the implemented policy. If 24 hours is the final decision, this is acceptable, but the deviation from the original recommendation should be acknowledged or the issue should document why 24 hours was chosen instead of 12.
|
|
||
| Notes for implementation | ||
|
|
||
| - MVP can be implemented with Next.js + Firebase (Auth + Firestore) and deployed to Vercel. |
There was a problem hiding this comment.
The documentation mentions Firebase as an implementation option in MVP.md line 579, but ARCHITECTURE.md (line 4) and ROADMAP.md (line 6) specify Supabase as the chosen stack. Line 580 states "Exact stack and schema are finalized in ARCHITECTURE.md", which clarifies that Supabase is the final choice. However, this reference to Firebase in the MVP.md may cause confusion. Consider either removing the Firebase reference or clarifying that while Firebase was considered, Supabase was ultimately chosen (as documented in ARCHITECTURE.md).
| - MVP can be implemented with Next.js + Firebase (Auth + Firestore) and deployed to Vercel. | |
| - MVP stack uses Next.js with Supabase as the backend (see ARCHITECTURE.md), deployed to Vercel. Firebase (Auth + Firestore) was an earlier/alternative option but is not used in the current architecture. |
| 'no_show_tutor' | ||
| ); | ||
|
|
||
| 5.2 reference tables |
There was a problem hiding this comment.
The schema section numbering appears to have a formatting inconsistency. Line 227 shows "5.2 reference tables" without the expected section header formatting (it should be "## 5.2 reference tables" or follow the pattern used elsewhere). This same issue appears throughout section 5 where subsection headers are not consistently formatted as SQL comments or markdown headers. Consider ensuring consistent formatting for readability.
| format: https://wa.me/ | ||
| <E164_NUMBER>?text=<urlencoded_message> |
There was a problem hiding this comment.
The wa.me link format on lines 1033-1034 has a line break that makes it unclear. The format should be shown as a single line: "https://wa.me/E164_NUMBER?text=urlencoded_message" or formatted more clearly with backticks to indicate it's a URL template. The current formatting with E164_NUMBER on a separate line could be confusing for implementers.
| format: https://wa.me/ | |
| <E164_NUMBER>?text=<urlencoded_message> | |
| format: `https://wa.me/<E164_NUMBER>?text=<urlencoded_message>` |
| ### 1) Clone and install | ||
|
|
||
| ```bash | ||
| git clone <your-repo-url> |
There was a problem hiding this comment.
The git clone command "git clone " contains a placeholder. This should be updated with the actual repository URL or use a more generic instruction like "git clone [your-forked-repo-url]" to make it clear this is user-specific.
| git clone <your-repo-url> | |
| git clone [your-forked-repo-url] |
|
|
||
| alternatively compute remaining sessions on the fly (simpler, but more query complexity) | ||
|
|
||
| 6) security model and RLS policies |
There was a problem hiding this comment.
Section 6 header on line 443 is not formatted as a proper markdown heading. It should be "## 6) security model and RLS policies" to match the document structure shown in the table of contents (line 16). This formatting inconsistency makes the document harder to navigate. The same issue affects section numbering throughout the schema and security sections where headers are plain text instead of markdown headers.
| @@ -0,0 +1,15 @@ | |||
| Summary | |||
There was a problem hiding this comment.
The PR description states "Closes #7 (T1.2 add issue templates and PR template) ← only if you actually added PR template too". A PR template has been added at .github/pull_request_template.md, so this issue should be closed without the conditional note. Consider removing the "only if you actually added PR template too" qualifier since the PR template is indeed included.
| - name: CorvEd WhatsApp Support | ||
| url: https://wa.me/<YOUR_NUMBER_IN_E164> | ||
| about: For student/parent support requests (non-technical). |
There was a problem hiding this comment.
The placeholder URL "https://wa.me/<YOUR_NUMBER_IN_E164>" contains a non-functional placeholder. This should be updated with an actual phone number in E.164 format (e.g., +92XXXXXXXXXX) or removed before launch. Consider using an environment-based approach or documenting this as a setup step.
| - name: CorvEd WhatsApp Support | |
| url: https://wa.me/<YOUR_NUMBER_IN_E164> | |
| about: For student/parent support requests (non-technical). |
| create or replace function public.tutor_update_session( | ||
| p_session_id uuid, | ||
| p_status public.session_status_enum, | ||
| p_notes text | ||
| ) | ||
| returns void | ||
| language plpgsql | ||
| security definer | ||
| as $$ | ||
| declare | ||
| v_match_id uuid; | ||
| v_tutor_id uuid; | ||
| begin | ||
| select s.match_id into v_match_id | ||
| from public.sessions s | ||
| where s.id = p_session_id; | ||
|
|
||
| if v_match_id is null then | ||
| raise exception 'session not found'; | ||
| end if; | ||
|
|
||
| select m.tutor_user_id into v_tutor_id | ||
| from public.matches m | ||
| where m.id = v_match_id; | ||
|
|
||
| if v_tutor_id <> auth.uid() and not public.is_admin(auth.uid()) then | ||
| raise exception 'not authorized'; | ||
| end if; | ||
|
|
||
| -- allow only certain statuses for tutor-side updates | ||
| if p_status not in ('done','no_show_student','no_show_tutor') and not public.is_admin(auth.uid()) then | ||
| raise exception 'invalid status transition'; | ||
| end if; | ||
|
|
||
| update public.sessions | ||
| set status = p_status, | ||
| tutor_notes = p_notes, | ||
| updated_by_user_id = auth.uid(), | ||
| updated_at = now() | ||
| where id = p_session_id; | ||
|
|
||
| -- package decrement logic is best handled in application or a dedicated function | ||
| -- MVP approach: maintain sessions_used by counting sessions with consuming statuses within package window | ||
| -- or update package.sessions_used here via deterministic query. | ||
|
|
||
| insert into public.audit_logs(actor_user_id, action, entity_type, entity_id, details) | ||
| values (auth.uid(), 'tutor_update_session', 'session', p_session_id, jsonb_build_object('status', p_status)); | ||
|
|
||
| end; | ||
| $$; |
There was a problem hiding this comment.
The public.tutor_update_session RPC is defined as security definer and relies on auth.uid() for authorization, but it does not guard against the unauthenticated case where auth.uid() is null. In PL/pgSQL, the condition if v_tutor_id <> auth.uid() and not public.is_admin(auth.uid()) then evaluates to false when auth.uid() is null, so an unauthenticated caller (using only the public anon key) can bypass this check and update any session whose id they know, modifying status and tutor_notes without being the assigned tutor or an admin. You should explicitly require a non-null authenticated user (e.g., fail fast when auth.uid() is null) and, if this function is meant to be tutor-only, restrict EXECUTE on it to the authenticated role and enforce that only the assigned tutor (or admin) can call it.
Summary
Testing
Closes
Closes T0.1 finalize MVP rules in docs/PRODUCT.md #2 (T0.1 finalize MVP rules in docs/PRODUCT.md)
Closes T0.2 choose stack and hosting #3 (T0.2 choose stack and hosting)
Closes T0.3 define data model in docs/ARCHITECTURE.md #4 (T0.3 define the data model in docs/ARCHITECTURE.md)
Closes T1.2 add issue templates and PR template #7 (T1.2 add issue templates and PR template) ← only if you actually added PR template too
Closes T1.4 environment setup guide in README (local dev steps) #9 (T1.4 environment setup guide in README)