Support share expiry options, metadata and owner-updatable share links; frontend active-share handling and tests - #113
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the share-link feature to support selectable expiries (2h/7d), persists share ownership/metadata in Redis, and adds an authenticated owner-only update flow that refreshes TTLs; the frontend is updated to select expiry, remember the active share id, and auto-update the active share after sync.
Changes:
- Add Redis-backed share metadata (
share_meta:{id}) and TTL refresh helpers; enforce fixed share-id length. - Add
PUT /api/share/<share_id>for authenticated, owner-only share updates with rate limiting and Redis-unavailable handling. - Frontend: expiry selector + persist active share id + auto-update active share after successful sync; expand backend test coverage for new routes and behaviors.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
app/services/share_service.py |
Adds share metadata read/write/TTL-refresh helpers; enforces fixed share id length and strips share_expiry from stored payload. |
app/routes/share.py |
Adds expiry options, requires login for share creation, adds authenticated owner-only update endpoint, returns 503 when Redis is unavailable. |
frontend/share.js |
Adds expiry selector UI, persists active share id, and implements updateActiveShare() + getActiveShareId(). |
frontend/sync.js |
Calls updateActiveShare() after successful sync to keep an active share link updated. |
tests/backend/test_share.py |
Updates share id generation test and adds invalid-length share id coverage. |
tests/backend/test_share_routes.py |
New route-level tests for create/update ownership, TTL handling, rate limiting, Redis-unavailable behavior, and metadata TTL refresh failures. |
Comments suppressed due to low confidence (1)
frontend/share.js:33
- New
getActiveShareId()/updateActiveShare()behavior (localStorage persistence + conditional clearing on 403/404 + credentials-included PUT) isn’t covered by the existing frontend unit tests (which already use JSDOM). Adding a small test suite for these helpers would help prevent regressions (e.g., not attempting update when no active id, clearing the key on 403/404, and preserving it on transient failures).
const ACTIVE_SHARE_ID_KEY = 'activeShareId';
export function getActiveShareId() {
return localStorage.getItem(ACTIVE_SHARE_ID_KEY);
}
export async function updateActiveShare(gradesData) {
const shareId = getActiveShareId();
if (!shareId || !gradesData) return { attempted: false, ok: false, status: null };
const res = await fetch(`/api/share/${shareId}`, {
method: 'PUT',
headers: { 'Content-Type': 'application/json' },
credentials: 'include',
body: JSON.stringify(gradesData)
});
if (!res.ok && (res.status === 404 || res.status === 403)) {
localStorage.removeItem(ACTIVE_SHARE_ID_KEY);
}
return { attempted: true, ok: res.ok, status: res.status };
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request enhances the sharing functionality by allowing users to update existing share links and select between two expiry options: 2 hours or 7 days. Key changes include the addition of a PUT endpoint for share updates, ownership verification using Redis-stored metadata, and frontend integration to automatically update active shares during synchronization. Review feedback recommends removing a redundant fallback in the rate-limiting logic and simplifying the TTL refresh process to ensure more consistent error handling when links expire.
I am having trouble creating individual review comments. Click here to see my feedback.
app/routes/share.py (106)
The or request.remote_addr fallback is redundant here because the code already checks if requester_student_no is None at line 103 and returns a 401 response if it is. Therefore, requester_student_no is guaranteed to be a valid value at this point.
rate_limit_subject = requester_student_no
app/routes/share.py (140-145)
The logic for refreshing the share TTL can be simplified and made more robust.
- The check
if share_ttl not in SHARE_EXPIRY_OPTIONS.values():is unnecessarily restrictive. If a share was validly created with a TTL that is no longer in the current UI options (e.g., due to a configuration change), the owner should still be able to update and refresh it using its original TTL. - If
refresh_share_metadata_ttlfails, it typically means the metadata key has already expired in Redis. Returning a 404 (Not Found) is more consistent with the earlier check at line 126 than a 409 (Conflict).
Note: If you apply this change, remember to update the corresponding test case test_share_update_returns_409_when_metadata_ttl_refresh_fails in tests/backend/test_share_routes.py to expect a 404 status code.
share_ttl = metadata.get('ttl_seconds', current_app.config['SHARE_TTL'])
if not refresh_share_metadata_ttl(redis_client, share_id, share_ttl):
return jsonify({'error': 'Link expired or not found'}), 404
Motivation
Description
2h/7d) and validation in the share creation flow and frontend UI selection, and persisted the chosen TTL to Redis when creating a share.share_meta:{id}withcreator_student_no,created_at, andttl_seconds, plus helperswrite_share_metadata,read_share_metadata, andrefresh_share_metadata_ttlinshare_service.py.PUT /api/share/<share_id>endpoint that requires login, enforces owner-only updates, rate-limits updates, refreshes metadata TTL, and rewrites shared data; also madeGET /api/share/<id>and create/update paths return 503 when Redis is unavailable.localStorage, sending credentials with the create call, and anupdateActiveSharecall that attempts to auto-update the active share after sync; added helpergetActiveShareId.Testing
tests/backend/test_share.pyand added new route tests intests/backend/test_share_routes.pywhich exercise creation, 2h/7d TTL handling, owner authorization, rate limiting, Redis-unavailable handling, and metadata TTL refresh behavior.pytest tests/backend -qand all tests in the modified test suite passed.Codex Task