-
-
Notifications
You must be signed in to change notification settings - Fork 884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #3558 NoMongo: Postgres Migration Support for Settings in orgDashboard #3597
base: develop-postgres
Are you sure you want to change the base?
Fixes #3558 NoMongo: Postgres Migration Support for Settings in orgDashboard #3597
Conversation
WalkthroughThis pull request overhauls several GraphQL mutations and queries along with associated React components and utility interfaces. Mutation signatures are updated to use single input objects with expanded return fields, while queries are revised with new parameter names and additional data. Component logic is adjusted to accommodate these API changes, including simplified deletion checks and updated mock data. Minor utility updates include interface field reordering and a language code correction. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLAPI
Client->>GraphQLAPI: Send CreateCustomField(input: {organizationId, name, type})
GraphQLAPI-->>Client: Return customField { id, name, type, organizationId }
Client->>GraphQLAPI: Send DeleteCustomField(input: { id })
GraphQLAPI-->>Client: Return { success }
sequenceDiagram
participant User
participant Frontend
participant GraphQLAPI
User->>Frontend: Initiate organization deletion
Frontend->>GraphQLAPI: Send DeleteOrganization(input: { id })
GraphQLAPI-->>Frontend: Return deletion success status
Frontend-->>User: Notify deletion result
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/components/OrgSettings/General/OrgUpdate/OrgUpdateMocks.ts (1)
202-202
:⚠️ Potential issueFix typo in error key name.
There's a typo in the error key name.
- erorr: new Error('Mock Graphql Updating Organization Error'), + error: new Error('Mock Graphql Updating Organization Error'),src/components/OrgSettings/General/OrgUpdate/OrgUpdate.tsx (1)
375-375
:⚠️ Potential issueRemove code coverage disable statement.
Pipeline failure indicates presence of code coverage disable statement. Please remove it and add appropriate tests.
🧹 Nitpick comments (4)
src/GraphQl/Mutations/mutations.ts (1)
762-763
: Avoid partial exports scattered across files.
Comment suggests these constants belong in another file. Consider moving all organization-related mutations intoOrganizationMutations.ts
to avoid confusion and keep the re-export consistent.src/GraphQl/Queries/Queries.ts (1)
1015-1016
: Comment about updatingORGANIZATION_CUSTOM_FIELDS
.
You added a note indicating you’re updating a custom fields query. Make sure to remove or update this comment once the new query is finalized and integrated so it doesn’t become stale.src/GraphQl/Mutations/OrganizationMutations.ts (2)
238-248
: LGTM: Updated custom field creation mutation.The mutation has been correctly updated to use the new input type and return structure for Postgres compatibility.
Update the JSDoc comment above the mutation to reflect the new input structure:
/** * GraphQL mutation to add a custom field to an organization. * - * @param organizationId - The ID of the organization where the custom field is being added. - * @param type - The type of the custom field (e.g., String, Number). - * @param name - The name of the custom field. + * @param input - The input object containing organization ID, field type, and name. * @returns The added organization custom field object. */
261-264
: LGTM: Updated custom field deletion mutation.The mutation has been correctly simplified to use a single ID parameter and return a success status.
Update the JSDoc comment above the mutation to reflect the new parameter structure:
/** * GraphQL mutation to remove a custom field from an organization. * - * @param organizationId - The ID of the organization from which the custom field is being removed. - * @param customFieldId - The ID of the custom field to be removed. + * @param id - The ID of the custom field to be removed. * @returns The success status of the operation. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/GraphQl/Mutations/OrganizationMutations.ts
(2 hunks)src/GraphQl/Mutations/mutations.ts
(4 hunks)src/GraphQl/Queries/PlugInQueries.ts
(1 hunks)src/GraphQl/Queries/Queries.ts
(7 hunks)src/components/OrgSettings/General/DeleteOrg/DeleteOrg.tsx
(3 hunks)src/components/OrgSettings/General/OrgUpdate/OrgUpdate.tsx
(4 hunks)src/components/OrgSettings/General/OrgUpdate/OrgUpdateMocks.ts
(1 hunks)src/components/UpdateSession/UpdateSession.tsx
(1 hunks)src/utils/interfaces.ts
(1 hunks)src/utils/languages.ts
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/utils/languages.ts (2)
Learnt from: bandhan-majumder
PR: PalisadoesFoundation/talawa-admin#3593
File: src/utils/languages.ts:20-20
Timestamp: 2025-02-14T02:30:04.613Z
Learning: The language code for Spanish must remain as 'sp' (not 'es') to match the language enum defined in talawa-api backend.
Learnt from: bandhan-majumder
PR: PalisadoesFoundation/talawa-admin#3593
File: src/utils/languages.ts:20-20
Timestamp: 2025-02-14T02:30:04.613Z
Learning: The language code for Spanish must be 'sp' (not 'es') to match the schema in talawa-api backend.
🪛 GitHub Actions: PR Workflow
src/GraphQl/Queries/Queries.ts
[warning] 1-1: Code style issues found. Run Prettier with --write to fix.
src/GraphQl/Mutations/mutations.ts
[warning] 1-1: Code style issues found. Run Prettier with --write to fix.
src/components/OrgSettings/General/OrgUpdate/OrgUpdate.tsx
[error] 1-1: File contains code coverage disable statement. Please remove it and add the appropriate tests.
🔇 Additional comments (20)
src/GraphQl/Mutations/mutations.ts (3)
44-59
: Consolidate organization update inputs.
Changing from multiple parameters to a singleMutationUpdateOrganizationInput!
is a clean approach. The expanded return fields look correct. Ensure that all consumers now pass an appropriate input object and handle the richer response in the UI or other layers as needed.
78-106
: Verify the breadth of returned user fields.
You've added multiple fields (e.g.,birthDate
,isEmailAddressVerified
). Make sure the user’s privacy settings allow retrieving these fields. Confirm that only authorized clients can access PII like birth date and phone numbers, and that data privacy laws and guidelines are followed.
252-257
: Return ofname
upon deletion.
Returning the organization name alongside its ID here is a user-friendly choice for confirmations. Just confirm thatdeleteOrganization
is only callable by authorized roles, as revealing the name of a deleted org could be sensitive in some edge cases.src/GraphQl/Queries/Queries.ts (7)
6-35
: ExpandedCURRENT_USER
query.
You’ve added numerous PII fields (e.g.,birthDate
,mobilePhoneNumber
) to the query. Verify that your access controls protect these fields and that you truly need all these fields in the client. Exposing large amounts of user data can introduce privacy/security risks.
498-524
: Use of a single input object forORGANIZATIONS_LIST
.
Switching to$input: QueryOrganizationInput!
is consistent with a streamlined GraphQL design. Confirm that any existing calls are updated to supply this new input object.
707-724
: RefinedORGANIZATION_EVENT_LIST
query.
The query now takes$input: QueryOrganizationInput!
and yieldsevents
with important fields. Ensure the front-end is updated to handle the new structure, particularly thestartAt
andendAt
fields.
936-954
: UpdatedGET_COMMUNITY_DATA
with social media URLs.
Including the extended fields (e.g.,X
for Twitter) is beneficial. Double-check that these fields match your back-end schema naming conventions and that your UI is prepared to handle them (e.g., nulls if any fields are missing).
[approve]
974-980
: Renamed query:CommunitySessionTimeout
.
Switching fromGET_COMMUNITY_SESSION_TIMEOUT_DATA_PG
toGET_COMMUNITY_SESSION_TIMEOUT_DATA
is clear. Ensure that any references in the client or docs are updated to the new name.
1017-1031
: NewORGANIZATION_CUSTOM_FIELDS
query.
Retrieving custom fields viaorganizationCustomFields
is a good approach for clarity. Confirm your type definitions, particularly thetype
field, are aligned with the back end (string constants vs. enum).
1032-1045
: NewGET_ORGANIZATION_EVENTS
query.
Exposing organization events at the top level with$input: QueryOrganizationInput!
is consistent with other queries. Verify you handle absent or empty events gracefully in the UI.src/components/OrgSettings/General/DeleteOrg/DeleteOrg.tsx (2)
59-60
: LGTM: Query parameter renamed correctly.The variable name change from
isSampleOrganizationId
toid
aligns with the Postgres migration changes.
85-85
: LGTM: Updated mutation input structure.The mutation input structure has been updated to use an input object, which aligns with the Postgres migration changes.
src/components/OrgSettings/General/OrgUpdate/OrgUpdateMocks.ts (1)
14-14
: LGTM: Updated field name for Postgres compatibility.The change from
_id
toid
correctly aligns with the Postgres database schema.src/components/UpdateSession/UpdateSession.tsx (1)
78-85
: LGTM: Simplified session timeout data access.The changes improve code readability by:
- Simplifying the data access path
- Using clearer null/undefined checks
src/GraphQl/Queries/PlugInQueries.ts (1)
366-376
:❓ Verification inconclusive
Verify the query structure with backend schema.
The query has been updated to match the Postgres schema, but there are a few points to verify:
- The change from
ID!
toString!
type- The use of input object pattern
input: { id: $id }
- The new fields being requested
🏁 Script executed:
#!/bin/bash # Description: Check if the query structure matches the schema # Look for other similar queries to verify the pattern rg -A 5 "query.*\(\$id: String!\)"Length of output: 35
Verify Query Structure Consistency with Backend Schema
The update to
IS_SAMPLE_ORGANIZATION_QUERY
reflects the objectives for the Postgres migration by:
- Changing the parameter from
isSampleOrganizationId: ID!
toid: String!
- Adopting the input object pattern (
input: { id: $id }
)- Expanding the requested fields to include
id
,name
,description
, andisSampleOrganization
Before merging, please manually verify:
- That the backend GraphQL schema now expects a
String!
type for theid
argument.- The use of the input object (
input: { id: $id }
) is consistent with similar queries.- All requested fields in the query are defined in the backend schema.
src/components/OrgSettings/General/OrgUpdate/OrgUpdate.tsx (4)
46-46
: Component name follows React naming convention.The function name has been updated from
orgUpdate
toOrgUpdate
to follow React component naming convention.
23-34
: Interface structure aligns with Postgres schema.The new
MutationUpdateOrganizationInput
interface properly defines the structure for the organization update mutation, making all fields optional exceptid
.
159-163
: Good addition of form validation.Added validation ensures that required fields are provided before submitting the update.
389-389
: Improved loading state handling.The save button is now properly disabled during the update operation and shows a loading state.
Also applies to: 392-392
src/utils/interfaces.ts (1)
1276-1281
: Interface updated to match Postgres schema.The
InterfaceCustomFieldData
has been properly updated with:
- Added
id
field- Added
organizationId
field- Reordered properties to match schema
@@ -86,7 +108,7 @@ export const ORGANIZATION_CONNECTION_LIST = gql` | |||
where: { name_contains: $filter } | |||
first: $first | |||
skip: $skip | |||
orderBy: $orderBy | |||
orderBy: $order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential mismatch in variable usage.
This line uses orderBy: $order
, but your query arguments define $orderBy: OrganizationOrderByInput
. Either rename the variable or adjust the query input to avoid confusion or runtime errors.
- orderBy: $order
+ orderBy: $orderBy
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
orderBy: $order | |
orderBy: $orderBy |
@@ -17,7 +17,7 @@ const languages = [ | |||
country_code: 'in', | |||
}, | |||
{ | |||
code: 'sp', | |||
code: 'es', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert language code for Spanish.
Per the retrieved learnings, the back end expects 'sp'
rather than 'es'
. Changing this to 'es'
may break the language selection or cause mismatches in the API. Revert to 'sp'
per the existing schema.
- code: 'es',
+ code: 'sp',
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
code: 'es', | |
code: 'sp', |
// const canDelete = getItem('SuperAdmin'); | ||
const canDelete = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: Restore the super admin check for organization deletion.
Removing the super admin check creates a significant security vulnerability by allowing any user to delete organizations.
-// const canDelete = getItem('SuperAdmin');
-const canDelete = true;
+const canDelete = getItem('SuperAdmin');
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// const canDelete = getItem('SuperAdmin'); | |
const canDelete = true; | |
const canDelete = getItem('SuperAdmin'); |
What kind of change does this PR introduce?
Feature update: Updated queries and mutations to align with the recently migrated
develop-postgres
branch oftalawa-api
.Issue Number:
Fixes #3558
Snapshots/Videos:
https://www.loom.com/share/a26e865e314d48b48336f1f9193c64c9?sid=7a7e550d-190e-488e-9c73-fbf1304c3e07
If relevant, did you update the documentation?
No
Summary
This PR updates the GraphQL schema to support the latest changes in the
develop-postgres
branch. The queries and mutations have been modified accordingly to ensure compatibility with the new structure.Does this PR introduce a breaking change?
No, this PR does not introduce breaking changes.
Checklist
CodeRabbit AI Review
Test Coverage
Other information
N/A
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Refactor