Postgres Migration Support for Plugins and Plugin store in orgDashboard #3745
Conversation
WalkthroughThis pull request updates documentation, interface definitions, GraphQL schema and queries, and React component code to standardize the plugin identifier field from Changes
Sequence Diagram(s)sequenceDiagram
participant Store as AddOnStore.tsx
participant Helper as PluginHelper.fetchInstalled
participant Server as External Server
Store->>Helper: Call fetchInstalled()
Helper->>Server: HTTP GET http://localhost:${process.env.PORT}/installed
Server-->>Helper: Return JSON data
Helper-->>Store: Return installed plugins data
Store->>Store: Process data with debug logs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🪛 LanguageTooldocs/docs/auto-docs/components/AddOn/core/AddOnStore/AddOnStore/functions/getInstalledIds.md[uncategorized] ~7-~7: A punctuation mark might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (11)
✨ 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
|
![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
🔭 Outside diff range comments (2)
schema.graphql (1)
1815-1821:⚠️ Potential issueSchema change requires special permission.
While the change from
_idtoidin thePlugintype is correct and aligns with standard naming conventions, the pipeline indicates this is a sensitive file that requires special permission to modify.To proceed with this change, request the 'ignore-sensitive-files-pr' label to be added to the PR.
src/components/AddOn/support/services/Plugin.helper.ts (1)
1-1: 🛠️ Refactor suggestionRefine ESLint disable statement.
The current ESLint disable statement is too broad. Consider:
- Using more specific disable rules
- Adding proper TypeScript types instead of
anyReplace the broad disable with specific ones and add proper types:
-/* eslint-disable @typescript-eslint/no-explicit-any */ +import { InterfacePluginHelper } from '../../../types/AddOn/interface'; + +type StoreData = { + plugins: InterfacePluginHelper[]; +}; + +type PluginLink = { + name: string; + url: string; +};Then update the method signatures:
- fetchStore = async (): Promise<any> => { + fetchStore = async (): Promise<StoreData> => { - fetchInstalled = async (): Promise<any> => { + fetchInstalled = async (): Promise<StoreData> => { - generateLinks = (plugins: any[]): { name: string; url: string }[] => { + generateLinks = (plugins: InterfacePluginHelper[]): PluginLink[] => {🧰 Tools
🪛 GitHub Actions: PR Workflow
[error] 1-1: File contains eslint-disable statements. ESLint-disable check failed. Exiting with error.
🧹 Nitpick comments (1)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (1)
94-106: Optimize plugin filtering performance.The current filtering implementation could be improved for better performance:
- Move case conversion outside the filter loop
- Consider memoizing filtered results
Here's an optimized version:
const filterPlugins = ( plugins: InterfacePluginHelper[], searchTerm: string, ): InterfacePluginHelper[] => { - console.log('Plugin is triggered: ', plugins); if (!searchTerm) { return plugins; } + const lowerSearchTerm = searchTerm.toLowerCase(); return plugins.filter((plugin) => - plugin.pluginName?.toLowerCase().includes(searchTerm.toLowerCase()), + plugin.pluginName?.toLowerCase().includes(lowerSearchTerm), ); };Consider using React.useMemo to cache filtered results:
const filteredPlugins = React.useMemo( () => filterPlugins(data?.getPlugins || [], searchText), [data?.getPlugins, searchText] );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/docs/auto-docs/components/AddOn/support/services/Plugin.helper/classes/default.md(1 hunks)docs/docs/auto-docs/types/AddOn/interface/interfaces/InterfacePluginHelper.md(1 hunks)schema.graphql(1 hunks)src/GraphQl/Queries/PlugInQueries.ts(1 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(5 hunks)src/components/AddOn/support/services/Plugin.helper.ts(1 hunks)src/types/AddOn/interface.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/docs/auto-docs/components/AddOn/support/services/Plugin.helper/classes/default.md
🧰 Additional context used
🪛 GitHub Actions: PR Workflow
src/components/AddOn/support/services/Plugin.helper.ts
[error] 1-1: File contains eslint-disable statements. ESLint-disable check failed. Exiting with error.
schema.graphql
[error] 1-1: Unauthorized change/delete attempt on 'schema.graphql'. To override this, apply the 'ignore-sensitive-files-pr' label.
🔇 Additional comments (5)
src/GraphQl/Queries/PlugInQueries.ts (1)
12-12: LGTM! The field name change aligns with schema updates.The change from
_idtoidin thePLUGIN_GETquery matches the schema changes and follows standard naming conventions.src/types/AddOn/interface.ts (1)
34-34: LGTM! Interface update matches schema changes.The change from
_idtoidinInterfacePluginHelpercorrectly aligns with the schema and query changes, maintaining consistency across the codebase.src/components/AddOn/support/services/Plugin.helper.ts (1)
22-24: LGTM! Dynamic port configuration improves flexibility.Using
process.env.PORTinstead of hardcoding the port number is a good practice for configuration management.docs/docs/auto-docs/types/AddOn/interface/interfaces/InterfacePluginHelper.md (1)
27-33: LGTM! Documentation update aligns with codebase standardization.The renaming of
_idtoidis well-documented and consistent with modern TypeScript/JavaScript conventions. This change is part of a coordinated effort to standardize plugin identifiers across the codebase.src/components/AddOn/core/AddOnStore/AddOnStore.tsx (1)
52-52: LGTM! Consistent plugin identifier updates.The updates to use
idinstead of_idare consistent with the interface changes and follow the standardization effort across the codebase.Also applies to: 55-55, 185-185, 223-223
|
palisadoes
added
the
ignore-sensitive-files-pr
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
CONTRIBUTING.md (8)
32-34: Typo Correction in Code of Conduct Section
On line 34, there is a typo: “Respones must be respectful.” should be corrected to “Responses must be respectful.”- No one should fear voicing their opinion. Respones must be respectful. + No one should fear voicing their opinion. Responses must be respectful.🧰 Tools
🪛 LanguageTool
[style] ~32-~32: This expression usually appears with a “please” in front of it.
Context: ...DUCT.md) to understand what this means. Let us know immediately if you have unacceptable ex...(INSERT_PLEASE)
106-134: Jest Testing Section – Add Language Specifiers to Fenced Code Blocks
The Jest testing instructions are clear; however, the fenced code blocks (e.g., on lines 110, 114, 118, and 122) lack language identifiers. Specifying a language (such asbash) will improve syntax highlighting and help comply with markdown linting rules (MD040).Example diff for one of the blocks:
- ``` - npm run test path/to/test/file - ``` + ```bash + npm run test path/to/test/file + ```Please apply similar updates to all fenced code blocks in this section.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
110-110: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
136-153: Vitest Testing Section – Ensure Fenced Code Blocks Include Language Tags
Similar to the Jest section, the Vitest testing instructions (lines 136–153) would benefit from specifying language identifiers for the fenced code blocks. This enhances readability and conforms to markdown guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
140-140: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
155-164: Combined Testing and Coverage Instructions – Add Code Block Languages
In the “Combined testing and coverage” section, the fenced code blocks currently do not specify a language. Consider adding a language tag (e.g.,bash) to these blocks to improve clarity and adhere to markdown best practices.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
159-159: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
166-177: Test Code Coverage Section – Heading and Code Block Enhancements
The “#### Test Code Coverage:” heading ends with a colon, which is flagged by markdown style guidelines (MD026). It is recommended to remove the trailing punctuation for consistency. Additionally, ensure that any fenced code blocks within this multi-line section include a language specifier.- #### Test Code Coverage: + #### Test Code Coverage🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
167-167: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
172-172: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
179-187: Testing Individual Files Instructions – Code Block Language Tags
The instructions for testing individual files contain multiple fenced code blocks (lines 181–183 and 185–187) that currently lack language identifiers. Adding these (for example, usingbash) will ensure better readability and compliance with markdown guidelines.🧰 Tools
🪛 LanguageTool
[grammar] ~179-~179: The operating system from Apple is written “macOS”.
Context: ...r packages can be found for Windows and MacOS. - The currently acceptable cover...(MAC_OS)
🪛 markdownlint-cli2 (0.17.2)
183-183: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
188-199: Code Coverage Account Setup – Address Empty Image Links and Add Language Tags
In the “Creating your code coverage account” section, please note the following:
- Some image links (e.g., lines 195–197) are empty (i.e.
with empty parentheses), which may affect rendering. Consider adding descriptive alt text or removing the empty links if they are unnecessary.- As with previous sections, ensure all fenced code blocks in this area include a language specifier to improve formatting.
200-205: Git Workflow Instructions – Clarity Check
The instructions for adding, committing, and pushing changes (lines 200–205) are clear. Please confirm that they align with the project’s conventions. If any additional context is needed for new contributors, consider providing links to further documentation on git workflows.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
202-202: No empty links
null(MD042, no-empty-links)
203-203: No empty links
null(MD042, no-empty-links)
205-205: No empty links
null(MD042, no-empty-links)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CONTRIBUTING.md(2 hunks)docs/docs/auto-docs/components/AddOn/support/services/Plugin.helper/classes/default.md(3 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(4 hunks)src/components/AddOn/support/services/Plugin.helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/AddOn/core/AddOnStore/AddOnStore.tsx
- src/components/AddOn/support/services/Plugin.helper.ts
- docs/docs/auto-docs/components/AddOn/support/services/Plugin.helper/classes/default.md
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[grammar] ~179-~179: The operating system from Apple is written “macOS”.
Context: ...r packages can be found for Windows and MacOS. - The currently acceptable cover...
(MAC_OS)
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
103-103: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
110-110: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
140-140: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
159-159: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
167-167: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
172-172: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
183-183: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
202-202: No empty links
null
(MD042, no-empty-links)
203-203: No empty links
null
(MD042, no-empty-links)
205-205: No empty links
null
(MD042, no-empty-links)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
CONTRIBUTING.md (1)
100-101: Test Coverage Requirement Clarification
The “General” section now clearly states the expectation for 100% test coverage and the consequences if the minimum isn’t met. Ensure that new contributors understand these requirements.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
CONTRIBUTING.md (7)
71-71: Suggestion: Use Present Tense for ClarityThe line currently reads “For Talawa Admin, we had employed the following branching strategy…” Consider rephrasing to “we employ” (or a similar present-tense construction) to clearly communicate an ongoing practice rather than a past action.
107-136: Suggestion: Specify Language for Fenced Code Blocks (Jest Testing Section)In the “#### Jest Testing” section, several fenced code blocks (e.g., around commands) lack language identifiers. Specifying a language (such as
bashfor shell commands) can improve readability and assist tooling with syntax highlighting.For example, change:
- ``` - npm run test path/to/test/file - ``` + ```bash + npm run test path/to/test/file + ```🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
110-110: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
137-154: Suggestion: Specify Language for Fenced Code Blocks (Vitest Testing Section)Similarly, in the “#### Vitest Testing” section, the fenced code blocks do not include a language identifier. Adding one (e.g.,
bash) will enhance readability and maintain consistency throughout the document.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
140-140: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
156-165: Suggestion: Specify Language for Fenced Code Blocks (Combined Testing and Coverage Section)The instructions under “#### Combined testing and coverage” include fenced code blocks that lack language specifiers. Adding an appropriate language (such as
bash) to these blocks is recommended for consistency and clarity.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
159-159: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
167-167: Style Recommendation: Remove Trailing Punctuation in HeadingThe heading “#### Test Code Coverage:” includes a trailing colon. For a cleaner markdown style and to adhere to common guidelines (e.g., MD026), consider removing the colon.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
167-167: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
178-178: Correction: Use Correct Branding for Apple’s OSIn the line mentioning “...packages can be found for Windows and MacOS,” update “MacOS” to the correct casing “macOS” to reflect the standard brand styling.
183-190: Suggestion: Add Language Identifiers for Fenced Code Blocks (Testing Individual Files Section)The “Testing Individual Files” section also contains fenced code blocks without a language specifier. For consistency and improved syntax highlighting, please add an identifier (e.g.,
bash) to these blocks.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
183-183: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
src/components/AddOn/core/AddOnEntry/AddOnEntry.spec.tsx (3)
146-170: Good mock setup for GraphQL mutation testing.The test now uses a consistent UUID for organization ID and properly mocks the GraphQL mutation response. This approach effectively tests the component's interaction with the API.
However, the UUID is hardcoded in multiple places throughout the tests. Consider defining it as a constant at the top of the file to improve maintainability.
- mockID = 'cd3e4f5b-6a7c-8d9e-0f1a-2b3c4d5e6f7a';Add at the top of the file:
const TEST_ORG_UUID = 'cd3e4f5b-6a7c-8d9e-0f1a-2b3c4d5e6f7a';Then use
TEST_ORG_UUIDinstead of the hardcoded value in all places.
208-292: Well-structured test for state changes with re-rendering.This test effectively demonstrates how the component should behave when receiving updated props after a mutation. The re-rendering with updated
uninstalledOrgsarray properly tests the conditional rendering logic.Consider adding a test case for error handling when the mutation fails, as this would improve test coverage of exceptional cases.
test('Handles mutation errors correctly', async () => { mockID = TEST_ORG_UUID; // Assuming you've added the constant as suggested earlier const errorMocks = [ { request: { query: UPDATE_INSTALL_STATUS_PLUGIN_MUTATION, variables: { pluginId: '1', orgId: TEST_ORG_UUID, }, }, error: new Error('An error occurred'), }, ]; render( <MockedProvider mocks={errorMocks} addTypename={false}> <Provider store={store}> <BrowserRouter> <I18nextProvider i18n={i18nForTest}> <ToastContainer /> <AddOnEntry {...props} /> </I18nextProvider> </BrowserRouter> </Provider> </MockedProvider>, ); const btn = screen.getByTestId('AddOnEntry_btn_install'); await userEvent.click(btn); // Wait for error toast to appear await waitFor(() => { expect(screen.getByText(/error/i)).toBeInTheDocument(); }); });
249-261: Consider refactoring repetitive test setup.The test rendering code is duplicated between the initial render and re-render. Consider extracting a helper function to reduce repetition:
const renderComponent = (props, mocks) => { return render( <MockedProvider mocks={mocks} addTypename={false}> <Provider store={store}> <BrowserRouter> <I18nextProvider i18n={i18nForTest}> <ToastContainer /> <AddOnEntry {...props} /> </I18nextProvider> </BrowserRouter> </Provider> </MockedProvider> ); }; // Then use: // const { rerender } = renderComponent(props, mocks); // rerender(renderComponent({...props, uninstalledOrgs: [TEST_ORG_UUID]}, mocks).container.innerHTML);Also applies to: 273-288
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CONTRIBUTING.md(2 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.spec.tsx(4 hunks)src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/AddOn/core/AddOnEntry/AddOnEntry.tsx
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~73-~73: Loose punctuation mark.
Context: ...hed to the master branch: - develop: For unstable code and bug fixing - `mas...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...bug fixing - master: Where the stable production ready code lies. This is our default branch. ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~179-~179: The operating system from Apple is written “macOS”.
Context: ...r packages can be found for Windows and MacOS. - The currently acceptable cover...
(MAC_OS)
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
103-103: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
110-110: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
140-140: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
159-159: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
167-167: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
172-172: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
183-183: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
202-202: No empty links
null
(MD042, no-empty-links)
203-203: No empty links
null
(MD042, no-empty-links)
205-205: No empty links
null
(MD042, no-empty-links)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (8)
CONTRIBUTING.md (2)
74-74: Approval: Updated Default Branch NameThe branch name has been correctly updated from
maintomasteras per the PR objectives. Please ensure consistency across the project where the default branch is referenced.🧰 Tools
🪛 LanguageTool
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...bug fixing -master: Where the stable production ready code lies. This is our default branch. ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
100-101: Feedback on Test Coverage GuidelinesThe reformatting to a numbered list and the explicit requirement for 100% test coverage are clear improvements. The message—that pull requests not meeting the minimum coverage levels will not be accepted—is explicit. Consider a slight rewording for a softer tone if desired, but this is acceptable as is.
src/components/AddOn/core/AddOnEntry/AddOnEntry.spec.tsx (6)
8-8: Good addition of 'within' from testing library.Adding the
withinutility is a good practice as it allows more precise element targeting within specific containers, making your tests more robust when multiple similar elements exist on the page.
23-23: Import aligns with implementation changes.This import brings in the mutation needed for plugin installation status updates, consistent with the Postgres migration support for plugins being implemented in this PR.
139-139: Helpful comment for test clarity.The comment explains the relationship between the empty
uninstalledOrgsarray and the button text, improving test readability.
186-205: Improved element targeting and detailed test assertions.Using
within()to find elements scoped to a specific container is a best practice. The test properly verifies:
- Initial button state
- User interaction (button click)
- Toast message appearance
- Button text after interaction
The explanatory comment on line 204 is particularly helpful in clarifying the component's behavior.
267-271: Properly testing toast notifications.The test correctly waits for the toast notification to appear, which is important for verifying user feedback. This is a good practice for testing asynchronous UI updates.
289-291: Comprehensive testing of conditional rendering.Testing that the button text changes to "Install" after re-rendering with updated props effectively validates the component's conditional rendering logic. This is crucial for ensuring the UI reflects the current state correctly.
|
@MayankJha014 Please fix the first comment so that each issue listed automatically closes. The PR_GUIDELINES.md file has details. Please make sure all checks pass. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
CONTRIBUTING.md (7)
100-101: Test Coverage Guidelines: Consistency and Clarity
The updated guidelines clearly state the goal of achieving 100% test coverage and the consequences for falling below the minimum. For better overall consistency, consider aligning the formatting with the numbered lists used in other sections.
107-136: Jest Testing Section: Code Block Language and Minor Typo
The expanded Jest testing instructions are very helpful and detailed. It is recommended to specify a language (e.g.,bash) in the fenced code blocks to improve syntax highlighting and readability. Additionally, there is a minor typo—"broswer" should be corrected to "browser" (line 126).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
110-110: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
137-154: Vitest Testing Section: Enhance Code Block Readability
The Vitest testing guidelines are clear and well-structured. To further improve the presentation, consider adding a language specifier (such asbash) to the fenced code blocks for better clarity and consistency with the Jest section.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
140-140: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
156-165: Combined Testing and Coverage: Clear Instructions with a Minor Enhancement
This section effectively outlines how to execute both test suites and generate combined code coverage reports. As with previous sections, specifying a language in the fenced code blocks would improve readability.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
159-159: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
167-180: Test Code Coverage Details: Minor Typos and Style Adjustments
The instructions on generating the test coverage report are comprehensive. Please correct "tablular" to "tabular" (line 177) and update "MacOS" to "macOS" (line 179) to adhere to standard terminology.🧰 Tools
🪛 LanguageTool
[grammar] ~179-~179: The operating system from Apple is written “macOS”.
Context: ...r packages can be found for Windows and MacOS. - The currently acceptable cover...(MAC_OS)
🪛 markdownlint-cli2 (0.17.2)
167-167: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
172-172: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
183-189: Testing Individual Files: Clear and Actionable Guidance
The procedures for testing individual files and generating their coverage reports are clearly described. For consistency and improved formatting, consider adding a language specifier to the fenced code blocks.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
183-183: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
192-200: Code Coverage Account Setup: Comprehensive and Accessible
The guidance provided for setting up a Codecov account and viewing coverage reports is thorough. The use of images enriches the instructions. It would be beneficial to verify that the images render correctly and to consider adding descriptive alt text for improved accessibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CONTRIBUTING.md(2 hunks)
🧰 Additional context used
🪛 LanguageTool
CONTRIBUTING.md
[uncategorized] ~73-~73: Loose punctuation mark.
Context: ...hed to the master branch: - develop: For unstable code and bug fixing - `mas...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...bug fixing - master: Where the stable production ready code lies. This is our default branch. ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~179-~179: The operating system from Apple is written “macOS”.
Context: ...r packages can be found for Windows and MacOS. - The currently acceptable cover...
(MAC_OS)
🪛 markdownlint-cli2 (0.17.2)
CONTRIBUTING.md
103-103: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
110-110: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
114-114: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
118-118: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
122-122: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
128-128: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
140-140: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
144-144: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
148-148: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
152-152: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
159-159: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
163-163: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
167-167: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
172-172: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
183-183: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
187-187: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Check Python Code Style
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (1)
CONTRIBUTING.md (1)
71-74: Branching Strategy Update: Verify Consistency of Primary Branch Name
The branching strategy section now correctly refers tomasteras the default production branch instead ofmain, which aligns with the updated conventions outlined in the PR objectives. Please double-check that all related documentation and scripts elsewhere in the repository also reflect this change.🧰 Tools
🪛 LanguageTool
[uncategorized] ~73-~73: Loose punctuation mark.
Context: ...hed to themasterbranch: -develop: For unstable code and bug fixing - `mas...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~74-~74: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...bug fixing -master: Where the stable production ready code lies. This is our default branch. ...(EN_COMPOUND_ADJECTIVE_INTERNAL)
|
Sir I am working on it to cover them in test coverage |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (2)
401-489: Remove debugging statement and consider refactoring the test mock.The test case is well-structured and properly verifies that installed plugins are correctly displayed. However, there are two issues:
- The debug statement on line 477 should be removed before merging as it's only intended for development.
- The test duplicates mocking logic that's already present earlier in the file.
Apply this diff to improve the test:
test('correctly identifies and displays installed plugins', async () => { vi.resetModules(); vi.mock('components/AddOn/support/services/Plugin.helper', () => ({ __esModule: true, default: vi.fn().mockImplementation(() => ({ fetchStore: vi.fn().mockResolvedValue([]), fetchInstalled: vi.fn().mockResolvedValue([ { id: '1', _id: '1', pluginName: 'Plugin 1', pluginDesc: 'Description 1', pluginCreatedBy: 'User 1', }, { id: '3', _id: '3', pluginName: 'Plugin 3', pluginDesc: 'Description 3', pluginCreatedBy: 'User 3', }, ]), })), })); const mocks = [ { request: { query: PLUGIN_GET, }, result: { data: { getPlugins: [ { _id: '1', pluginName: 'Plugin 1', installed: true, }, { _id: '2', pluginName: 'Plugin 2', installed: false, }, { _id: '3', pluginName: 'Plugin 3', installed: true, }, ], }, }, }, ]; render( <ApolloProvider client={client}> <Provider store={store}> <BrowserRouter> <I18nextProvider i18n={i18nForTest}> <MockedProvider mocks={mocks} addTypename={false}> <AddOnStore /> </MockedProvider> </I18nextProvider> </BrowserRouter> </Provider> </ApolloProvider>, ); await wait(); const installedTab = screen.getByText('Installed'); fireEvent.click(installedTab); await wait(); - screen.debug(); // Debug output - await waitFor(() => { expect(screen.getAllByText('Plugin 1')).toHaveLength(1); expect(screen.getAllByText('Plugin 3')).toHaveLength(1); }); await waitFor(() => { expect( screen.queryByText('Plugin 2', { exact: false }), ).not.toBeInTheDocument(); }); });
404-425: Consider consolidating mock implementations.There are multiple instances of mocking the same Plugin.helper service in this file (lines 22-70, 72-78, and now 404-425). Consider refactoring to centralize these mocks or use a beforeEach hook with different mock implementations for specific tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx(2 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/AddOn/core/AddOnStore/AddOnStore.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test Application
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (3)
2-2: Good practice: AddedwaitForfor asynchronous testing.The addition of
waitForfrom@testing-library/reactis appropriate for handling asynchronous UI updates in testing.
410-411: Proper handling of transition from_idtoidfield.The test mock data includes both
_idandidproperties for each plugin, which suggests a transition or compatibility layer in the codebase. This is a good practice to ensure backward compatibility while migrating to the new field name.Also applies to: 417-418
479-488: Good practice: Using appropriate testing patterns.The test correctly uses
waitForfor asynchronous assertions and properly verifies both positive cases (installed plugins are displayed) and negative cases (non-installed plugins are not displayed).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (1)
31-32:⚠️ Potential issue❓ Verification inconclusive
Inconsistent plugin ID field naming
The existing tests use
_idfor plugin identification, but the new test usesid. This inconsistency could lead to test failures when schema changes are applied. Ensure all tests use the same field name consistently throughout the codebase.Use consistent field naming across all plugin mocks:
Also applies to: 38-39, 48-49, 55-56, 196-198, 241-243, 289-291, 356-358, 365-367, 449-451, 458-460
🏁 Script executed:
#!/bin/bash # Check for inconsistencies in plugin ID field naming across test files rg -A 1 "_id|id: " --type ts --type tsx --glob "**/AddOn/**/*.spec.*" | grep -E "(_id|id): "Length of output: 122
Action Required: Verify Consistent Plugin ID Field Naming
The initial verification script failed because Ripgrep did not recognize the “tsx” file type, making it hard to conclude if there’s an inconsistency in the plugin ID field naming. Please run the updated verification command below to check all test files for a consistent use of the plugin identifier field (ideally
_idas used in most mocks):#!/bin/bash # Re-run verification with custom file type support for ts and tsx files rg --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -A 1 "_id|id:" --glob "**/AddOn/**/*.spec.*" | grep -E "(_id|id):"Once you’ve confirmed that all plugin mocks (e.g., in
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsxat lines 31-32, 38-39, 48-49, 55-56, 196-198, 241-243, 289-291, 356-358, 365-367, 449-451, 458-460) consistently use_id(or your agreed-upon naming), update the tests if necessary.
🧹 Nitpick comments (4)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (4)
405-405: Remove redundant commentThis comment "Add this test to AddOnStore.spec.tsx" is redundant since we're already in that file and should be removed.
- // Add this test to AddOnStore.spec.tsx
490-505: Direct function testing versus component interactionThis test directly implements and tests the
getStorePluginsfunction rather than testing the component's behavior through user interactions, which is less effective for a component test.Consider refactoring this test to focus on component behavior rather than implementation details:
- // Directly test the getStorePlugins function by extracting its logic - const getStorePlugins = async (): Promise<void> => { - let plugins: InterfacePluginHelper[] = - (await new PluginHelper().fetchStore()) as InterfacePluginHelper[]; - - const installIds = ( - (await new PluginHelper().fetchInstalled()) as InterfacePluginHelper[] - ).map((plugin: InterfacePluginHelper) => plugin.id); - - plugins = plugins.map((plugin: InterfacePluginHelper) => { - plugin.installed = (installIds || []).includes(plugin.id); - return plugin; - }); - - store.dispatch({ type: 'UPDATE_STORE', payload: plugins }); - }; - - // Call the function - await getStorePlugins(); + // Wait for component to load data and update store + await wait(); + + // Trigger a re-fetch (if there's a UI element to do so) + // For example: + // fireEvent.click(screen.getByTestId('refresh-button')); + + // Wait for the store update to happen + await wait();
498-501: Avoid mutating objects directlyThe test mutates the plugin objects directly by setting the
installedproperty, which could lead to unexpected side effects in testing.Use immutable patterns to create new objects instead of mutating existing ones:
- plugins = plugins.map((plugin: InterfacePluginHelper) => { - plugin.installed = (installIds || []).includes(plugin.id); - return plugin; - }); + plugins = plugins.map((plugin: InterfacePluginHelper) => ({ + ...plugin, + installed: (installIds || []).includes(plugin.id) + }));
493-497: Simplify type castingThere's redundant type casting that makes the code harder to read.
Simplify the type casting in the fetchInstalled call:
- const installIds = ( - (await new PluginHelper().fetchInstalled()) as InterfacePluginHelper[] - ).map((plugin: InterfacePluginHelper) => plugin.id); + const installedPlugins = await new PluginHelper().fetchInstalled() as InterfacePluginHelper[]; + const installIds = installedPlugins.map(plugin => plugin.id);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (2)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (2)
408-434: Potential mock conflict with existing implementationThere's already a mock for
PluginHelperat lines 26-74 and another at lines 76-82. Adding a third mock here might cause unexpected behavior as the last mock will override previous ones.Consider refactoring the test setup to use a more maintainable approach:
- vi.mock('components/AddOn/support/services/Plugin.helper', () => ({ - __esModule: true, - default: vi.fn().mockImplementation(() => ({ - fetchStore: vi.fn().mockResolvedValue([ - { - id: '1', // This ID matches one in the installed plugins - pluginName: 'Plugin 1', - pluginDesc: 'Description 1', - pluginCreatedBy: 'User 1', - }, - { - id: '2', // This ID doesn't match any installed plugin - pluginName: 'Plugin 2', - pluginDesc: 'Description 2', - pluginCreatedBy: 'User 2', - }, - ]), - fetchInstalled: vi.fn().mockResolvedValue([ - { - id: '1', // This ID matches one of the store plugins - pluginName: 'Installed Plugin 1', - pluginDesc: 'Installed Description 1', - pluginCreatedBy: 'User 3', - }, - ]), - })), - })); + // Use vi.mocked to update the mock implementation for this specific test + const mockPluginHelper = vi.mocked(PluginHelper); + mockPluginHelper.mockImplementation(() => ({ + fetchStore: vi.fn().mockResolvedValue([ + { + id: '1', // This ID matches one in the installed plugins + pluginName: 'Plugin 1', + pluginDesc: 'Description 1', + pluginCreatedBy: 'User 1', + }, + { + id: '2', // This ID doesn't match any installed plugin + pluginName: 'Plugin 2', + pluginDesc: 'Description 2', + pluginCreatedBy: 'User 2', + }, + ]), + fetchInstalled: vi.fn().mockResolvedValue([ + { + id: '1', // This ID matches one of the store plugins + pluginName: 'Installed Plugin 1', + pluginDesc: 'Installed Description 1', + pluginCreatedBy: 'User 3', + }, + ]), + }));
20-24: Good type imports updateThe addition of proper type imports enhances type safety in the test code.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (2)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (2)
20-24: Import changes look good for the new test case.The addition of
InterfacePluginHelpertype andPluginHelperimport supports the new test functionality properly.
31-32: Standardized plugin identifier field from_idtoid.These changes consistently update all references to use
idinstead of_id, which aligns with the standardization effort mentioned in the PR summary.Also applies to: 38-39, 48-49, 55-56, 197-198, 241-242, 289-290, 356-357, 365-366
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (1)
494-506: Consider adding a unit test case for edge conditions.While the current test covers the basic scenario, consider adding test cases for edge conditions such as:
- Empty arrays of plugins
- Plugins with missing or null IDs
- Multiple installed plugins with the same ID
This would ensure the function is robust against unexpected data.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx(8 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/AddOn/core/AddOnStore/AddOnStore.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (5)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (5)
20-24: Appropriate import additions for the new test.The added imports for
InterfacePluginHelperandPluginHelperare properly structured and necessary for the new test case.
31-39: Consistent update of plugin identifier field names.The plugin identifier field has been properly standardized from
_idtoidthroughout the test file, which maintains consistency with the changes in the related files.Also applies to: 48-56, 197-198, 241-242, 289-290, 356-366
406-532: Well-implemented test case with proper mock isolation.This new test effectively verifies that plugins are correctly marked as installed based on their IDs. The implementation follows best practices by:
- Properly resetting mocks before the test with
vi.resetAllMocks()- Using a consistent mocking strategy with
vi.mocked(PluginHelper).mockImplementation(...)- Testing a specific piece of functionality in isolation
- Including clear assertions that verify the expected behavior
This approach addresses the previous concerns about mock conflicts in the test file.
493-509: Well-structured utility function extraction.The
getStorePluginsfunction properly extracts and tests the core functionality that marks plugins as installed based on their IDs. This approach allows for clear validation of the specific business logic without unnecessary complexity.
514-528: Well-structured expectations with precise assertion.The test assertions correctly verify that the store's dispatch was called with the expected payload, checking both that the plugin with ID '1' is marked as installed and the plugin with ID '2' is not. This precision in assertions helps ensure the test accurately validates the functionality.
|
@MayankJha014 Please fix the first comment so that each issue listed automatically closes. The PR_GUIDELINES.md file has details. Please use the PR template as best you can. Please fix the failed check. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (1)
449-461: Consider extracting utility function from test implementation.The logic that maps installed plugins to store plugins is well-implemented but appears to duplicate functionality that might exist in the actual component. Consider extracting this to a utility function or documenting that this is replicating component logic for testing purposes.
// Consider moving this logic to a utility function if it's used elsewhere + // This replicates the plugin installation status mapping logic from the AddOnStore component const getStorePlugins = async (): Promise<void> => { let plugins: InterfacePluginHelper[] = (await new PluginHelper().fetchStore()) as InterfacePluginHelper[]; const installIds = ( (await new PluginHelper().fetchInstalled()) as InterfacePluginHelper[] ).map((plugin: InterfacePluginHelper) => plugin.id); plugins = plugins.map((plugin: InterfacePluginHelper) => { plugin.installed = installIds.includes(plugin.id); return plugin; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Application
🔇 Additional comments (3)
src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx (3)
20-24: Good addition of type imports for improved code safety.The new type imports for
InterfacePluginandInterfacePluginHelperenhance type safety and provide better documentation for what's being used in the tests.
31-32: Standardized plugin identifier from_idtoid.The changes consistently update all plugin identifiers from
_idtoidacross all mock data objects. This standardization improves consistency and aligns with the PR's goal of supporting Postgres migrations, which likely requires this field naming change.Also applies to: 38-39, 48-49, 55-56, 197-198, 241-242, 289-290, 356-357, 365-366
406-498: Well-structured test for plugin installation state logic.This test properly verifies that plugins are correctly marked as installed based on their IDs by:
- Properly mocking the plugin helper services with
vi.resetAllMocks()- Testing both installed and non-installed states
- Verifying the correct Redux action payload
The implementation follows best practices by isolating the test's mocks and properly cleaning up with
mockRestore().This refactoring addresses the previous review comment about mock conflicts by using
vi.resetAllMocks()and specific mock implementations for this test instead of relying on the global mock.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (1)
43-53: Add error handling for async operations.The
getStorePluginsfunction makes multiple async calls but lacks error handling. If eitherfetchStore()orfetchInstalled()fails, the function will throw unhandled exceptions, potentially causing the UI to break.const getStorePlugins = async (): Promise<void> => { + try { const plugins = (await new PluginHelper().fetchStore()) as InterfacePluginHelper[]; const installIds = ( (await new PluginHelper().fetchInstalled()) as InterfacePluginHelper[] ).map((p) => p.id); const updatedPlugins = markInstalledPlugins(plugins, installIds); store.dispatch({ type: 'UPDATE_STORE', payload: updatedPlugins }); + } catch (error) { + console.error('Failed to fetch plugins:', error); + // Consider adding user-friendly error handling here + } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/auto-docs/components/AddOn/core/AddOnStore/AddOnStore/functions/markInstalledPlugins.md(1 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx(9 hunks)src/components/AddOn/core/AddOnStore/AddOnStore.tsx(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/docs/auto-docs/components/AddOn/core/AddOnStore/AddOnStore/functions/markInstalledPlugins.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/AddOn/core/AddOnStore/AddOnStore.spec.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test Application
- GitHub Check: Check Python Code Style
- GitHub Check: Analyse Code With CodeQL (javascript)
🔇 Additional comments (3)
src/components/AddOn/core/AddOnStore/AddOnStore.tsx (3)
199-200: Good defensive programming practice.The addition of the nullish coalescing operator (
??) is a good practice to handle potentialundefinedvalues foruninstalledOrgs, preventing potential runtime errors.
237-245: Well-structured pure function extraction.Excellent job extracting the
markInstalledPluginsfunction. It follows good functional programming principles:
- It's pure (no side effects)
- It has a clear purpose and signature
- It handles a single responsibility
- It improves code maintainability and testability
The export also makes it available for unit testing, which is a good practice.
46-50: Remove debug console.log statements.These debug statements should be removed before merging to production as they:
- Add unnecessary noise to the console
- Could potentially expose sensitive information
- Are not following production logging best practices
Apply this diff to remove the debug statements.
|
@Cioppolo14 @palisadoes Sir Now its Passing all Test case. Is there anything needed in this PR. |
|
@tasneemkoushar @meetulr PTAL |
|
@MayankJha014 plugin support was removed from the postgres branch because the existing one in the admin isn't how we want plugins to work. The gsoc plugin project aims to implement a new plugin architecture from scratch. You can explore that If you're interested. You can contact me or @tasneemkoushar. |
|
@MayankJha014 this is no longer the requirement. Closing this PR & please go through the project idea list once. you will get the intent of the project. We are trying to change the whole architecture of the plugin in talawa. This Schema adheres to the old design which is not very streamlines. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #3745 +/- ##
====================================================
+ Coverage 86.05% 86.28% +0.22%
====================================================
Files 371 373 +2
Lines 9139 9231 +92
Branches 1925 1954 +29
====================================================
+ Hits 7865 7965 +100
+ Misses 909 885 -24
- Partials 365 381 +16 ☔ View full report in Codecov by Sentry. |
Plugin Management Progress
Relevant Links
🔗 Talawa API PR: #3294
🔗 Issue: #3559
https://www.loom.com/share/294daa2f2ae14a7c902611f75921c330?sid=48b14b86-c10a-495d-8f97-8e2103922fa9
Summary by CodeRabbit
Summary by CodeRabbit
Documentation
UPDATE_ORG_STATUS_PLUGIN_MUTATIONdocumentation.markInstalledPlugins(),getInstalledIds(), andgetStorePlugins()functions, detailing their parameters and return types.Refactor
Chores
Tests
AddOnEntrycomponent by incorporating mutation handling and validating button functionality.getInstalledIdsandmarkInstalledPluginsfunctions in the AddOnStore component.