enhance: introduce approval process for requested access to answer collections #4425
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive feature for managing collection sharing requests across the application. The changes span multiple components and services, adding new GraphQL queries and mutations to handle collection sharing. The implementation allows users to view, approve, and decline sharing requests for answer collections. The header component is updated to display notifications for pending requests, and a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
packages/graphql/src/services/resources.ts (3)
421-456: Consider returning an empty array instead ofnullfor consistency
The function returnsnullif the user is not found, while returning an empty array if the user simply has no requests might be more intuitive for the consumer of this API. This approach typically simplifies downstream code handling.
458-506: Resolve concurrency and error-handling edge cases
When multiple approval or denial actions for the same request occur nearly in parallel (e.g., race conditions), the code could silently fail to update or produce inconsistent states. Consider leveraging transactions or optimistic concurrency checks.
500-501: Complete the email notification for better user engagement
The TODO indicates the need to email requesters upon approval or denial. Providing timely status updates to users fosters a better experience.Would you like me to draft an email-sending utility function and open a separate PR for it?
packages/graphql/src/ops.ts (2)
2276-2276: Consider pagination or filtering options
getCollectionSharingRequestsreturns an unbounded list, which might cause performance concerns if requests grow large. Add filters or pagination arguments to ensure scalability.
2627-2631: Enrich the response type
SharingRequestResponsecurrently only returns thecollectionIdanduserId. Consider including a status or a reason to simplify handling on the client side.apps/frontend-manage/src/components/resources/answerCollections/CollectionSharingRequests.tsx (2)
14-16: Consider loading indicators
Currently, ifloadingis true, the component returns null. You might wish to show a spinner or skeleton to inform users that data is being fetched.
28-31: Empty state
When there are no outstanding requests, returningnullquietly omits the section. Consider providing a short phrase or a minimal UI cue (“No pending requests”) for clarity.apps/frontend-manage/src/components/common/Header.tsx (2)
30-30: Handle loading and error states for requests.When using
useQuery(GetCollectionSharingRequestsDocument), consider handling loading and error states (e.g.,loading,error) or providing a fallback UI. That will improve robustness and UX if the query or network call fails or takes longer than expected.
55-56: Guard against unexpected null data.
notification: (requestData?.getCollectionSharingRequests?.length ?? 0) > 0is a concise check. However, consider also showing a more explicit UI for error states or partial data. That can help differentiate between an empty list and a fetch error.packages/types/src/index.ts (1)
99-108: Validate the new type for possible expansions.
AnswerCollectionSharingRequestis introduced with required fields. Confirm whether some properties (likecollectionNameoruserEmail) might be optional in real-world scenarios. If so, consider making them optional to handle partial data or incomplete records gracefully.packages/graphql/src/graphql/ops/MApproveCollectionSharingRequest.graphql (1)
1-4: Well-structured mutation signature.The parameter types are clearly defined as required (
Int!andString!). Good for explicitness.packages/graphql/src/public/schema.graphql (1)
1530-1533: Consider enhancing the SharingRequestResponse typeWhile the current response type includes the basic fields needed to identify the request, consider adding a status field to explicitly indicate the outcome of the operation (e.g., APPROVED, DECLINED).
type SharingRequestResponse { collectionId: Int! userId: String! + status: SharingRequestStatus! } + +enum SharingRequestStatus { + APPROVED + DECLINED +}packages/graphql/src/ops.schema.json (3)
987-1078: LGTM! Consider adding field descriptions.The
AnswerCollectionSharingRequesttype is well-structured with appropriate field types and nullability constraints. However, adding descriptions for the type and its fields would improve schema documentation.Add descriptions to improve schema documentation:
{ "kind": "OBJECT", "name": "AnswerCollectionSharingRequest", - "description": null, + "description": "Represents a request from a user to access a specific answer collection", "fields": [ { "name": "collectionId", - "description": null, + "description": "Unique identifier of the answer collection being requested", // ... rest of the field }, // Add similar descriptions for other fields ] }
21273-21292: Consider adding filtering and pagination.The query returns all sharing requests without filtering or pagination, which could be problematic as the number of requests grows. Consider:
- Adding pagination parameters (e.g.,
first,after)- Adding filters (e.g., by status, date range)
- Adding sorting options
Example enhancement:
{ "name": "getCollectionSharingRequests", "args": [ + { + "name": "first", + "type": { "kind": "SCALAR", "name": "Int" } + }, + { + "name": "after", + "type": { "kind": "SCALAR", "name": "String" } + }, + { + "name": "status", + "type": { "kind": "ENUM", "name": "RequestStatus" } + } ], "type": { "kind": "OBJECT", - "name": "AnswerCollectionSharingRequest" + "name": "SharingRequestConnection" } }
23074-23117: LGTM! Consider adding operation status.The
SharingRequestResponsetype is appropriately structured for confirming operation success. Consider adding a status field to distinguish between different outcomes (e.g., approved, declined, already processed).{ "name": "SharingRequestResponse", "fields": [ + { + "name": "status", + "type": { + "kind": "NON_NULL", + "name": null, + "ofType": { + "kind": "ENUM", + "name": "RequestStatus" + } + } + }, // existing fields... ] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
apps/frontend-manage/src/components/common/Header.tsx(4 hunks)apps/frontend-manage/src/components/resources/AnswerCollections.tsx(2 hunks)apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionCollapsible.tsx(2 hunks)apps/frontend-manage/src/components/resources/answerCollections/CollectionSharingRequests.tsx(1 hunks)packages/graphql/src/graphql/ops/MApproveCollectionSharingRequest.graphql(1 hunks)packages/graphql/src/graphql/ops/MDeclineCollectionSharingRequest.graphql(1 hunks)packages/graphql/src/graphql/ops/QGetCollectionSharingRequests.graphql(1 hunks)packages/graphql/src/ops.schema.json(5 hunks)packages/graphql/src/ops.ts(13 hunks)packages/graphql/src/public/client.json(3 hunks)packages/graphql/src/public/schema.graphql(5 hunks)packages/graphql/src/public/server.json(3 hunks)packages/graphql/src/schema/mutation.ts(2 hunks)packages/graphql/src/schema/query.ts(2 hunks)packages/graphql/src/schema/resource.ts(2 hunks)packages/graphql/src/services/resources.ts(2 hunks)packages/i18n/messages/de.ts(3 hunks)packages/i18n/messages/en.ts(3 hunks)packages/types/src/index.ts(1 hunks)
🔇 Additional comments (41)
packages/graphql/src/services/resources.ts (2)
2-2: Use of typed structure is clear and readable
It's good to seeAnswerCollectionSharingRequestexplicitly imported, as it improves clarity and maintainability of the code.
421-456: Potential PII concern with exposing user emails
EachAnswerCollectionSharingRequeststructure includesuserEmail. Depending on your privacy requirements, ensure that returning user emails to the collection owner (or other consumers) is compliant with data protection policies.packages/graphql/src/ops.ts (11)
129-137: Ensure privacy for userEmail and userShortname fields
Storing and exposing userEmail can be considered personally identifiable information (PII). Verify that these fields are only exposed to authorized users and ensure proper access controls in your resolvers.
1104-1104: Mutation naming aligns with existing patterns
TheapproveCollectionSharingRequestmutation looks consistent with the established naming conventions. Ensure you have test coverage for all possible branches of this request approval process.
1125-1125: Mutation naming aligns with existing patterns
ThedeclineCollectionSharingRequestmutation is consistent with the related approval mutation, making it easy to maintain. Consider verifying coverage in your integration tests.
1245-1248: Confirm correct type usage foruserId
Double-check thatuserIdis intended to be a string (e.g., UUID or other form of ID). If your system uses numeric IDs, consider using an integer type for clarity.
1418-1421: Confirm correct type usage foruserId
As with the approval flow, verify thatuserIdis consistently typed across your schema and resolvers for the decline operation.
2929-2935: Mutation is well-structured
ApproveCollectionSharingRequestMutationfollows good naming practices and parameter definitions. Ensure robust testing to cover edge cases, like invalid collection IDs or user IDs.
3144-3150: Mutation is well-structured
DeclineCollectionSharingRequestMutationkeeps the API consistent. Consider whether a reason or note should also be captured, if relevant to your business logic.
3935-3938: Query looks good
GetCollectionSharingRequestsQueryprovides necessary data to the client. Confirm authorization checks are in place to prevent unauthorized viewing of requests.
4402-4402: Document for approving requests
ApproveCollectionSharingRequestDocumentis aligned with the new mutation. Ensure all calling components handle potential null returns appropriately.
4423-4423: Document for declining requests
DeclineCollectionSharingRequestDocumentis aligned with the new mutation. Similar to the approval, check for null returns and ensure error handling.
4522-4522: Query captures relevant info
GetCollectionSharingRequestsDocumentquery is consistent with the schema changes. Make sure that only authorized moderators or collection owners have access to these details.apps/frontend-manage/src/components/resources/answerCollections/AnswerCollectionCollapsible.tsx (2)
10-10: Flexible title prop type
Allowingstring | React.ReactNodeis a clean way to support customized titles. Good job!
21-21: Tailwind border classes
Addingborder-b border-b-gray-400aligns well with the design system. Just ensure this is consistent with the overall UI theme.apps/frontend-manage/src/components/resources/AnswerCollections.tsx (1)
6-6: Integration of CollectionSharingRequests
Pulling in theCollectionSharingRequestscomponent is a solid approach to modularizing the code for handling sharing requests. The placement looks appropriate, and the user flow remains clear.Also applies to: 21-21
packages/graphql/src/schema/resource.ts (3)
2-2: Expose user email carefully
The newAnswerCollectionSharingRequestTypeincludes a user’s email address, which is personally identifiable information (PII). Verify that exposinguserEmailis intentional and complies with privacy and data policies.
63-77: Well-structured object for sharing requests
DefiningAnswerCollectionSharingRequestwith separate fields for user details and collection identifiers keeps the schema consistent and clear.
79-90: SharingRequestResponse
The addition ofSharingRequestResponsealigns nicely with your mutations, guaranteeing a typed return object to confirm the outcome of approve/decline requests. Looks good.apps/frontend-manage/src/components/common/Header.tsx (2)
91-101: Confirm absolute positioning is well-tested across locales/devices.The “red dot” notification is positioned with absolute coordinates. Ensure this does not overlap or misalign in different browser sizes or languages, especially if labels or text lengths change.
9-9: Ensure query usage is consistent throughout.You have introduced the
GetCollectionSharingRequestsDocumentimport alongsideGetUserRunningLiveQuizzesDocumentandUser. Verify that all references to this newly imported query are updated consistently in the rest of the codebase, ensuring no outdated references remain.✅ Verification successful
Import and usage of GetCollectionSharingRequestsDocument is consistent
Based on the search results, the
GetCollectionSharingRequestsDocumentis consistently used across the codebase:
- It is properly defined in
packages/graphql/src/ops.ts- It is imported and used correctly in two files:
apps/frontend-manage/src/components/resources/answerCollections/CollectionSharingRequests.tsx: Used for fetching and managing collection sharing requestsapps/frontend-manage/src/components/common/Header.tsx: Used for fetching collection sharing requests dataAll references to the query are consistent, and there are no outdated or incorrect usages found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Locates all references to GetCollectionSharingRequestsDocument. rg --context 3 "GetCollectionSharingRequestsDocument"Length of output: 30557
packages/graphql/src/schema/query.ts (2)
56-60: Imports are logically consistent with the resource definitions.It’s good practice to keep resource-related imports grouped together. No immediate concerns spotted here.
837-843: Confirm data shape for user experience.
getCollectionSharingRequestsreturns an array ofAnswerCollectionSharingRequest. If an empty array is common, verify that the frontend gracefully handles an empty list scenario and that no ephemeral errors occur ifnullis returned.packages/graphql/src/schema/mutation.ts (3)
66-66: Confirmed import correctness.
SharingRequestResponseis imported for newly introduced sharing request mutations. Looks good.
1231-1246: Check user permissions before approving requests.
approveCollectionSharingRequestis guarded byasUserFullAccess. Verify that only authorized users can invoke this mutation and that it correctly logs or notifies the requestor about their request being approved if that is required.
1247-1260: Ensure consistent user feedback on declined requests.Similar to approving a request, handle any user notifications or logging upon declination. Confirm the caller’s permission checks and confirm no sensitive details are exposed in any error paths or logs.
packages/i18n/messages/en.ts (3)
237-239: Looks good – consistent with the rest of the translations.
1862-1862: Clarify the user data disclosure.The new sentence stating that the owner will see the shortname and email address is precise and well-defined. This transparency is beneficial and aligns with GDPR-like requirements to inform users about potential data exposure.
1878-1879: Useful addition for categorizing pending requests.Including 'sharingRequests' and 'unresolved' keys helps clarify and segment data for pending requests in the UI. This aligns with the new CollectionSharingRequests feature.
packages/i18n/messages/de.ts (3)
237-239: Translations align properly.The new keys "Akzeptieren", "Ablehnen", and "Nutzer" match their English counterparts.
1874-1874: Polished wording.Changing the phrasing from "anfragen" to "beantragen" is more natural in German and suits the formal style for requesting access.
1890-1891: Consistent with English references.The new keys "Zugriffs-Anfragen" (
sharingRequests) and "Unbearbeitet" (unresolved) integrate well with the user flow for pending requests.packages/graphql/src/graphql/ops/QGetCollectionSharingRequests.graphql (1)
1-9: Query structure is straightforward and aligns with naming conventions.This query appears correct, retrieving the relevant fields for each sharing request. Ensure that the server correctly resolves these fields to avoid
nullor incomplete data.packages/graphql/src/graphql/ops/MApproveCollectionSharingRequest.graphql (1)
5-12: Return fields are appropriate.Returning the
collectionIdanduserIdis sufficient to confirm a successful approval. Be sure to handle any relevant edge cases on the server side, like requests not found or already processed.packages/graphql/src/graphql/ops/MDeclineCollectionSharingRequest.graphql (1)
1-12: Well-structured mutation for declining collection sharing requestsThe mutation is correctly defined with proper typing and required parameters. The response includes the necessary fields to confirm which request was declined.
packages/graphql/src/public/client.json (1)
7-7: Consistent addition of collection sharing operation entriesThe new entries for ApproveCollectionSharingRequest, DeclineCollectionSharingRequest, and GetCollectionSharingRequests are properly formatted and include unique hash values, maintaining consistency with the existing entries.
Also applies to: 28-28, 127-127
packages/graphql/src/public/schema.graphql (2)
100-106: Well-defined type for collection sharing requestsThe AnswerCollectionSharingRequest type includes all necessary fields to identify both the collection and the requesting user. The use of required fields (!) ensures data completeness.
1012-1012: Verify error handling for sharing request operationsThe mutations and query are properly typed, but ensure proper error handling is implemented for cases such as:
- Requesting access to non-existent collections
- Approving/declining already processed requests
- Handling requests for users without proper permissions
Also applies to: 1033-1033, 1441-1441
packages/graphql/src/public/server.json (1)
7-7: Implementation of collection sharing operations looks good!The new GraphQL operations for managing collection sharing requests are well-structured with:
- Consistent naming patterns
- Proper type safety using non-nullable parameters
- Symmetrical mutation structures
- Comprehensive query fields for UI display
Let's verify the frontend implementation matches these operations:
Also applies to: 28-28, 127-127
✅ Verification successful
Frontend implementation matches the GraphQL operations
The verification confirms that the GraphQL operations are properly integrated in the frontend:
- The mutations and query are imported and used in
CollectionSharingRequests.tsx:
ApproveCollectionSharingRequestDocumentandDeclineCollectionSharingRequestDocumentare used withuseMutationGetCollectionSharingRequestsDocumentis used withuseQueryto fetch the requests- The operations are also properly cached and updated after mutations through Apollo cache updates
- The implementation in
Header.tsxalso usesGetCollectionSharingRequestsDocumentto display notifications🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the frontend components use these GraphQL operations correctly # Expected: Find usage of these operations in collection sharing related components # Check for mutation usage rg -A 3 "ApproveCollectionSharingRequest|DeclineCollectionSharingRequest" # Check for query usage rg -A 3 "GetCollectionSharingRequests"Length of output: 32963
packages/graphql/src/ops.schema.json (2)
13137-13181: LGTM! Consistent with approve mutation.The mutation maintains consistency with
approveCollectionSharingRequest, which is good for maintainability. The same documentation and error handling suggestions from the previous comment apply here.
11456-11500: Add documentation and verify error handling.The mutation structure is correct, but consider:
- Adding descriptions for the mutation and its arguments
- Verifying error handling for cases like:
- Request already approved/declined
- Collection or user no longer exists
- Approver lacks permission
Let's verify the error handling implementation:
| {requests.map((request) => { | ||
| return ( | ||
| <div | ||
| key={`sharing-request-${request.collectionId}-${request.userId}`} | ||
| className="flex flex-row items-center justify-between" | ||
| > | ||
| <div> | ||
| <div className="font-bold">{request.collectionName}</div> | ||
| <div className="text-sm">{`${t('shared.generic.user')}: ${request.userShortname} (${request.userEmail})`}</div> | ||
| </div> | ||
| <div className="flex flex-row gap-2"> | ||
| <Button | ||
| className={{ | ||
| root: 'h-7 border-green-600 hover:border-green-600 hover:text-green-800', | ||
| }} | ||
| onClick={() => | ||
| approveCollectionSharingRequest({ | ||
| variables: { | ||
| collectionId: request.collectionId, | ||
| userId: request.userId, | ||
| }, | ||
| update: (cache, { data }) => { | ||
| if (!data?.approveCollectionSharingRequest) return | ||
| const resolvedRequest = | ||
| data.approveCollectionSharingRequest | ||
|
|
||
| const queryData = cache.readQuery({ | ||
| query: GetCollectionSharingRequestsDocument, | ||
| }) | ||
| const previousRequests = | ||
| queryData?.getCollectionSharingRequests | ||
| if (!previousRequests) return | ||
|
|
||
| cache.writeQuery({ | ||
| query: GetCollectionSharingRequestsDocument, | ||
| data: { | ||
| getCollectionSharingRequests: | ||
| previousRequests.filter( | ||
| (r) => | ||
| r.collectionId !== | ||
| resolvedRequest.collectionId || | ||
| r.userId !== resolvedRequest.userId | ||
| ), | ||
| }, | ||
| }) | ||
| }, | ||
| }) | ||
| } | ||
| > | ||
| <FontAwesomeIcon icon={faCheck} /> | ||
| <div>{t('shared.generic.accept')}</div> | ||
| </Button> | ||
| <Button | ||
| className={{ | ||
| root: 'h-7 border-red-600 hover:border-red-600 hover:text-red-700', | ||
| }} | ||
| onClick={() => | ||
| declineCollectionSharingRequest({ | ||
| variables: { | ||
| collectionId: request.collectionId, | ||
| userId: request.userId, | ||
| }, | ||
| update: (cache, { data }) => { | ||
| if (!data?.declineCollectionSharingRequest) return | ||
| const resolvedRequest = | ||
| data.declineCollectionSharingRequest | ||
|
|
||
| const queryData = cache.readQuery({ | ||
| query: GetCollectionSharingRequestsDocument, | ||
| }) | ||
| const previousRequests = | ||
| queryData?.getCollectionSharingRequests | ||
| if (!previousRequests) return | ||
|
|
||
| cache.writeQuery({ | ||
| query: GetCollectionSharingRequestsDocument, | ||
| data: { | ||
| getCollectionSharingRequests: | ||
| previousRequests.filter( | ||
| (r) => | ||
| r.collectionId !== | ||
| resolvedRequest.collectionId || | ||
| r.userId !== resolvedRequest.userId | ||
| ), | ||
| }, | ||
| }) | ||
| }, | ||
| }) | ||
| } | ||
| > | ||
| <FontAwesomeIcon icon={faBan} /> | ||
| <div>{t('shared.generic.decline')}</div> | ||
| </Button> | ||
| </div> | ||
| </div> | ||
| ) | ||
| })} | ||
| </div> | ||
| </AnswerCollectionCollapsible> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance user feedback & error handling
The UI silently updates the Apollo cache on success. Consider confirming the user’s action with success messages or warnings if the mutation fails or partially applies. Also, a confirmation dialog for approval/decline might help avoid mistaken clicks.
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
answer-collection-approval
|
| Run status |
|
| Run duration | 13m 37s |
| Commit |
|
| Committer | Julius Schlapbach |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
174
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
New Features
Improvements
User Interface