Enhance share functionality with metadata, expiry options, and updates - #114
Conversation
…and-update-api feat: add owner-bound share metadata and share update flow
…and-update-api-kpnvqs Add configurable share expiry, share metadata and update endpoint; client-side active-share persistence
…and-update-api-8pykes Support share expiry options, metadata and owner-updatable share links; frontend active-share handling and tests
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to update existing share links and allows users to choose between different expiry durations (2 hours or 7 days). It implements metadata tracking to associate share links with their creators, ensuring that only the original owner can perform updates. Key changes include the addition of a PUT endpoint for share updates, new service methods for metadata management in Redis, and frontend logic to track and sync the active share ID. The review feedback identifies redundant conditional checks for the Redis client and student session data in the update route, and highlights a brittle validation check for expiry options that may cause issues if configuration options are modified in the future.
| if requester_student_no is None: | ||
| return jsonify({'error': 'Unauthorized'}), 401 | ||
|
|
||
| rate_limit_subject = requester_student_no or request.remote_addr |
There was a problem hiding this comment.
The fallback to request.remote_addr is redundant because requester_student_no is guaranteed to be truthy at this point due to the check at line 103.
| rate_limit_subject = requester_student_no or request.remote_addr | |
| rate_limit_subject = requester_student_no |
| if redis_client: | ||
| try: | ||
| limited, remaining, retry_after = is_rate_limited( | ||
| redis_client, | ||
| rate_limit_subject, | ||
| max_attempts=10, | ||
| window_seconds=60, | ||
| key_prefix='share_update' | ||
| ) | ||
| if limited: | ||
| resp = jsonify({ | ||
| 'error': f'更新分享過於頻繁，請在 {retry_after} 秒後再試', | ||
| }) | ||
| resp.headers['Retry-After'] = str(retry_after) | ||
| return resp, 429 | ||
| except Exception as exc: | ||
| logger.error(f'Share update rate limiter error: {exc}', exc_info=True) |
There was a problem hiding this comment.
The if redis_client: check is redundant because redis_client is already verified at line 99.
try:
limited, remaining, retry_after = is_rate_limited(
redis_client,
rate_limit_subject,
max_attempts=10,
window_seconds=60,
key_prefix='share_update'
)
if limited:
resp = jsonify({
'error': f'更新分享過於頻繁，請在 {retry_after} 秒後再試',
})
resp.headers['Retry-After'] = str(retry_after)
return resp, 429
except Exception as exc:
logger.error(f'Share update rate limiter error: {exc}', exc_info=True)| if share_ttl not in SHARE_EXPIRY_OPTIONS.values(): | ||
| return jsonify({'error': 'Share metadata state conflict'}), 409 |
There was a problem hiding this comment.
This check is brittle. If the available SHARE_EXPIRY_OPTIONS change in the future, existing valid share links might become impossible to update even if they haven't expired. Since this is an update to an existing link, we should respect the TTL already stored in the metadata rather than enforcing it against the current UI options.
There was a problem hiding this comment.
Pull request overview
This PR enhances the share-link feature by adding server-side share metadata, configurable expiry options, and an authenticated “update existing share” flow (with frontend auto-update after sync).
Changes:
- Backend: add share metadata storage, expiry selection (
2h/7d), and a new authenticatedPUT /api/share/<id>endpoint with TTL refresh + ownership checks. - Frontend: allow selecting share expiry on creation, persist an
activeShareId, and auto-update the active share after syncing grades. - Tests: extend share service tests and add a new backend route test suite covering create/update/expiry/auth/rate-limit/redis-unavailable cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/backend/test_share.py | Updates share-id generation test and validates stripping of share_expiry. |
| tests/backend/test_share_routes.py | Adds route-level tests for create/update flows, TTL behavior, expiry validation, authz, and error handling. |
| frontend/sync.js | Calls updateActiveShare() after a successful sync to keep shares current. |
| frontend/share.js | Adds expiry selector UI, stores activeShareId, and implements updateActiveShare() via PUT /api/share/<id>. |
| app/services/share_service.py | Introduces fixed-length share IDs, share metadata read/write, and metadata TTL refresh helpers. |
| app/routes/share.py | Adds expiry option handling, requires login for share creation, adds update endpoint with ownership + TTL refresh, and returns 503 when Redis is unavailable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| body: JSON.stringify(gradesData) | ||
| }); | ||
|
|
||
| if (!res.ok && (res.status === 404 || res.status === 403)) { |
There was a problem hiding this comment.
updateActiveShare only clears activeShareId on 404/403. If localStorage contains a malformed/old ID, the backend will return 400 (Invalid ID format) and the client will keep retrying on every sync, causing repeated failing requests/noise. Consider also removing activeShareId on 400 responses (and potentially other non-retryable statuses).
| if (!res.ok && (res.status === 404 || res.status === 403)) { | |
| if (!res.ok && (res.status === 400 || res.status === 403 || res.status === 404)) { |
| except Exception as exc: | ||
| logger.error(f'Error updating share: {exc}', exc_info=True) | ||
| return jsonify({'error': str(exc)}), 500 |
There was a problem hiding this comment.
The exception handler returns str(exc) to the client. This can leak internal details (e.g., backend errors, dependency messages) and makes it harder to keep error responses consistent. Prefer returning a generic 500 message (optionally with a request/error id) while keeping full details in logs.
…and-update-api-a4thl2 Share: add expiry options, metadata, update endpoint and frontend active-share sync
No description provided.