ServiceNow integration #1541
ServiceNow integration #1541
Conversation
Yibaebi
requested review from
future-pirate-king,
SmitGala and
avzz-19
and removed request for
future-pirate-king and
SmitGala
WalkthroughThis update introduces extensive ServiceNow integration support alongside adjustments for GitHub and JIRA. New adapter, model, and validation files for ServiceNow are added along with multiple new UI components and templates for organization and project settings integrations. Routes, controllers, and configuration files have been updated to support these capabilities, while obsolete components, tests, and SCSS files have been removed. Additional global type and translation updates are included, and Mirage factories and scenarios have been revised accordingly. Changes
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
🪧 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 (
|
Yibaebi
requested review from
future-pirate-king,
SmitGala and
avzz-19
and removed request for
avzz-19 and
future-pirate-king
Deploying irenestaging with
|
| Latest commit: |
d8644f4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://30fdc99d.irenestaging.pages.dev |
| Branch Preview URL: | https://pd-1715-service-now-integrat.irenestaging.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (38)
app/components/project-settings/integrations/jira-project/selected-project/index.scss (1)
1-4: Consider using standard padding values for better readabilityThe padding values (0.2587em and 0.4286em) are quite specific. Consider using more standard values like 0.25em and 0.5em for better readability and maintenance.
.threshold-risk-label { - padding: 0.2587em 0.4286em; + padding: 0.25em 0.5em; width: fit-content; }app/components/organization/integrations/service-now/validator.ts (1)
1-10: Consider enhancing validation rules with custom error messagesThe validation rules are correctly implemented for basic validation of ServiceNow credentials.
Consider enhancing the validator with custom error messages for better user experience:
export default { - instanceURL: validateFormat({ type: 'url' }), - username: validatePresence(true), - password: validatePresence(true), + instanceURL: validateFormat({ type: 'url', message: '{description} must be a valid URL' }), + username: validatePresence({ presence: true, message: '{description} is required' }), + password: validatePresence({ presence: true, message: '{description} is required' }), };app/components/project-settings/integrations/service-now/selected-threshold/index.scss (1)
6-15: Consider using a more appropriate CSS variable nameThe CSS variable
--project-settings-general-settings-jira-project-background-colorreferences Jira but is being used for ServiceNow integration styling. Consider creating a dedicated variable for ServiceNow or using a more generic variable name to avoid potential confusion during maintenance.- background-color: var( - --project-settings-general-settings-jira-project-background-color - ); + background-color: var( + --project-settings-integrations-divider-color + );app/components/organization/settings-wrapper/index.ts (1)
39-45: Consider using intl service for the "Integrations" label.For consistency with the organization-settings tab, consider using the translation service for the "Integrations" label instead of hardcoding it:
{ id: 'integrations', - label: 'Integrations', + label: this.intl.t('integrations'), route: 'authenticated.dashboard.organization-settings.integrations', currentWhen: 'authenticated.dashboard.organization-settings.integrations', },This ensures proper localization across the application.
app/components/project-settings/integrations/config-drawer/index.scss (1)
1-25: Consider responsive design for the drawer componentThe SCSS implementation is clean and follows good practices using CSS variables. However, the fixed width of 640px may cause layout issues on smaller screens.
Consider using relative units or media queries for responsive behavior:
.project-integrations-config-drawer-container { - width: 640px; + width: 100%; + max-width: 640px; height: 100%; overflow-y: auto; // rest of the styles... }Also, the CSS variables being used (lines 10-11, 21-22) suggest reusing styles from another context (
file-details-api-scan-*). It might be better to create dedicated variables for this component if they represent different semantic meanings.app/components/project-settings/integrations/index.hbs (1)
5-5: Data attribute name mismatch with component purpose.The data test attribute
data-test-projectSettings-generalSettings-rootmentions "generalSettings", but this component is specifically for "integrations". Consider renaming todata-test-projectSettings-integrations-rootfor clarity and consistency.- data-test-projectSettings-generalSettings-root + data-test-projectSettings-integrations-rootapp/components/project-settings/integrations/jira-project/index.scss (1)
20-22: Consider using more appropriate variable names for the integration context.The variables being used (
--file-details-api-scan-captured-apis-footer-border-colorand--file-details-api-scan-footer-border-radius) suggest they were designed for a different context. It would be better to define variables specific to the integration components.- border: 1px solid - var(--file-details-api-scan-captured-apis-footer-border-color); - border-radius: var(--file-details-api-scan-footer-border-radius); + border: 1px solid var(--integration-drawer-border-color); + border-radius: var(--integration-drawer-border-radius);Then ensure these new variables are defined in your CSS variables file with appropriate values.
app/controllers/authenticated/dashboard/project/settings/integrations.ts (1)
15-15: Consider potential string interpolation limitationEmbedding the model's packageName directly in the interpolated string with
this.intl.t('integration')might cause issues if the string needs to be positioned differently in other languages.Consider using a named parameter in the translation:
- title: `${this.intl.t('integration')} (${this.model.packageName})`, + title: this.intl.t('integrationWithPackage', { packageName: this.model.packageName }),This allows translators to control the positioning of the package name within the sentence structure of different languages.
app/components/project-settings/integrations/config-drawer/index.hbs (2)
25-33: Consider adding an aria-label for better accessibility.The close button uses an icon without text, which could be problematic for screen readers.
<AkIconButton class={{ab.classes.defaultIconBtn}} {{on 'click' dr.closeHandler}} + aria-label="Close drawer" data-test-prjSettings-integrations-configDrawer-closeBtn > <AkIcon @iconName='close' /> </AkIconButton>
17-24: Consider adding a fallback for drawer title.The drawer title is directly using the passed @drawerTitle without a fallback value.
<AkTypography @color='inherit' @variant='h5' data-test-prjSettings-integrations-configDrawer-title > - {{@drawerTitle}} + {{or @drawerTitle (t 'defaultDrawerTitle')}} </AkTypography>app/adapters/servicenow-config.ts (1)
18-23: Consider validating the risk threshold value before sending.There's no validation for the riskThreshold value before sending it to the server, which could lead to unexpected behavior.
updateRecord<K extends keyof ModelRegistry>( _: Store, _type: ModelSchema<K>, snapshot: Snapshot<K> ) { const { riskThreshold } = snapshot.record as ServiceNowConfigModel; + // Ensure riskThreshold is a valid number between 0 and 100 + const validRiskThreshold = Math.max(0, Math.min(100, Number(riskThreshold) || 0)); const url = this._buildURL(); return this.ajax(url, 'POST', { - data: { risk_threshold: riskThreshold }, + data: { risk_threshold: validRiskThreshold }, }); }app/components/project-settings/integrations/config-drawer/index.ts (2)
3-13: Add JSDoc comments to the interface for better developer experience.Adding documentation to the interface would improve developer experience and make the component's API clearer.
+/** + * Interface for the ProjectSettingsIntegrationsConfigDrawer component + */ export interface ProjectSettingsIntegrationsConfigDrawerSignature { Args: { + /** Controls whether the drawer is open */ openDrawer: boolean; + /** The title displayed in the drawer header */ drawerTitle: string; + /** Indicates if the save action is in progress */ loadingDrawerSaveAction: boolean; + /** Callback function triggered when the save button is clicked */ drawerSaveAction(): void; + /** Callback function triggered when the drawer should be closed */ closeDrawerHandler(): void; }; Blocks: { default: []; drawerContent: [] }; }
15-15: Add class documentation for better code maintainability.Adding documentation to the component class would improve maintenance and make the purpose clearer.
+/** + * A component that provides a configuration drawer for project integration settings. + * It includes a header with title, content area, and footer with save/cancel actions. + */ export default class ProjectSettingsIntegrationsConfigDrawerComponent extends Component<ProjectSettingsIntegrationsConfigDrawerSignature> {}app/components/organization/integrations/jira-account/index.hbs (2)
48-57: Use a standard value for autocomplete attribute.The autocomplete attribute on the API key field uses 'jira-password', which is not a standard value. Consider using 'new-password' or 'off' instead.
<AkTextField data-test-jiraAccount-apiKeyInput @placeholder={{concat 'Enter ' (t 'apiKey')}} @label={{t 'apiKey'}} @labelTypographyVariant='subtitle2' @labelTypographyColor='textPrimary' @type='password' @value={{this.changeset.password}} @error={{this.changeset.error.password}} - autocomplete='jira-password' + autocomplete='new-password' />
26-58: Consider moving inline styles to CSS classes.The component uses inline styles for width constraints. These would be better handled through CSS classes for consistency and maintainability.
- <form class='mt-2'> - <AkStack @direction='column' @spacing='2' {{style maxWidth='300px'}}> + <form class='mt-2'> + <AkStack @direction='column' @spacing='2' local-class='jira-form-fields'> <AkTextField data-test-jiraAccount-hostInput @placeholder={{concat 'Enter ' (t 'jiraHost')}} @value={{this.changeset.host}} @label={{t 'jiraHost'}} @labelTypographyVariant='subtitle2' @labelTypographyColor='textPrimary' @error={{this.changeset.error.host}} /> ... <AkButton data-test-jiraAccount-integrateBtn class='mt-2' @loading={{this.integrateJIRA.isRunning}} - {{style minWidth='190px'}} + local-class='integrate-button' {{on 'click' (perform this.integrateJIRA this.changeset)}} >Then add these classes to your component's CSS:
.jira-form-fields { max-width: 300px; } .integrate-button { min-width: 190px; }Also applies to: 60-68
tests/integration/components/project-settings/integrations/service-now-test.js (1)
115-116: Use a regex literal instead of the RegExp constructor.Replacing the RegExp constructor with a literal can simplify the code and improve performance.
- new RegExp('organization/settings/integrations', 'i') + /organization\/settings\/integrations/i🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
app/components/project-settings/integrations/index.ts (1)
24-31: Add explicit error handling or logging in the constructor's fetchProfile call.While the
fetchProfiletask handles errors internally, it might be clearer to log or explicitly handle errors here if the task fails, to give more insight during debugging.app/components/project-settings/integrations/service-now/index.ts (1)
105-129: Consider preventing multiple simultaneous fetches or concurrency overlap scenarios.When
fetchServiceNowConfigis triggered repeatedly, Ember Concurrency will queue or restart tasks by default, but if you anticipate user interactions causing multiple fetches, consider specifying a concurrency policy (like.restartable()) to avoid confusion or resource overhead.tests/integration/components/project-settings/integrations/github-project-test.js (3)
29-29: Nitpick: Rename function for clarity.
The function namecreatOrgRepoappears to be a typographical error. Consider renaming it tocreateOrgRepofor better readability.-const creatOrgRepo = (propsOverride = {}) => { +const createOrgRepo = (propsOverride = {}) => {
203-203: Grammar fix in test description.
Replace “atleast” with “at least” in the test name for correctness.- test('it renders select button with atleast one github repo ... + test('it renders select button with at least one github repo ...
199-199: Use a regex literal instead of the RegExp constructor.
Switching to a literal clarifies the pattern and addresses the lint warning.- new RegExp('organization/settings/integrations', 'i') + /organization\/settings\/integrations/i🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
app/components/project-settings/integrations/jira-project/selected-project/index.hbs (1)
8-8: Use “project” instead of “repo” for JIRA projects.
Since this template is for JIRA integration, using “repo” may be confusing.- {{t 'repo'}} + {{t 'project'}}tests/integration/components/project-settings/integrations/jira-project-test.js (2)
137-137: Use a regex literal instead of the RegExp constructor.
In keeping with best practices and static analysis suggestions, use a literal form.- new RegExp('organization/settings/integrations', 'i') + /organization\/settings\/integrations/i🧰 Tools
🪛 Biome (1.9.4)
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
141-141: Grammar fix in test description.
Replace “atleast” with “at least” for correctness.- test('it renders select button with atleast one JIRA project' ... + test('it renders select button with at least one JIRA project' ...app/components/organization/integrations/service-now/index.hbs (3)
9-19: Consider internationalizing the image alt text.
The alt text'service now icon'is hardcoded in English. You may want to use i18n for consistency, especially if your app supports multiple locales.- @imageAlt='service now icon' + @imageAlt={{t 'serviceNow.iconAlt'}}
29-49: Optional: Clarify placeholder guidance for instance URL.
Currently, users might be unsure if a full URL (e.g., "https://company.service-now.com") is needed. Consider specifying the expected format or including an example placeholder.- @placeholder={{t 'serviceNow.enterInstanceURL'}} + @placeholder={{t 'serviceNow.enterInstanceURLExample'}}
51-61: Autocomplete attribute verification.
Usingautocomplete='service-now-password'may not trigger password management in all browsers. Standard values likenew-passwordorcurrent-passwordare more widely recognized.translations/ja.json (4)
201-204: Strings are in English instead of Japanese.
These newly added confirmation messages are needing Japanese translations for consistent localization."removeServiceNow": "Are you sure you want to remove the ServiceNow config for this project?", "revokeServiceNow": "Are you sure you want to revoke ServiceNow Integration?", +// Suggest providing actual Japanese translations here for clarity and consistency.
609-612: Check if these entries require proper Japanese translations.
The new keys for integrating ServiceNow appear in English. If your app is expected to be fully localized in Japanese, consider a Japanese translation for consistency.
1455-1464: Newly added keys under "serviceNow" object are in English.
For complete localization, you might want to provide Japanese translations. Otherwise, users seeing Japanese in the rest of the UI may be confused by these English-only strings.
1465-1468: Additional ServiceNow strings in English.
These lines keep the integration text purely in English. If full localization is desired, please follow the same pattern used for other strings.app/components/organization/integrations/service-now/index.ts (1)
181-185: Use optional chaining for safer checks.
In line 185, checkingchangeset.errors && changeset.errors[0].validationcan be simplified with optional chaining for clarity:-if (changeset.errors && changeset.errors[0].validation) { +if (changeset.errors?.[0]?.validation) {🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/project-settings/integrations/github-project/index.hbs (1)
54-114: Config drawer implementation looks good.The configuration drawer is well-designed with:
- Clear title and structured content
- Repository and threshold selection dropdowns
- Appropriate conditional rendering based on GitHub repository availability
- Good use of data-test attributes for testing
One small suggestion: consider adding an explanatory note similar to the JIRA project implementation to improve user guidance.
app/components/project-settings/integrations/service-now/index.hbs (1)
1-5: Inconsistent test selector naming pattern.The data-test selector uses
projectSettings-genSettings-serviceNow-rootwhich differs from the pattern used in other integration components (projectSettings-integrations-*). Consider standardizing the test selector naming for consistency.- data-test-projectSettings-genSettings-serviceNow-root + data-test-projectSettings-integrations-serviceNow-rootapp/components/organization/integrations/jira-account/index.ts (1)
146-148: Improve error handling robustness.The error handling in the
integrateJIRAtask directly accesseschangeset.errors[0].validationwithout checking if the errors array exists or has elements, which could lead to runtime errors.Consider a more defensive approach:
- this.notify.error(changeset.errors[0].validation, ENV.notifications); + if (changeset.errors && changeset.errors.length > 0 && changeset.errors[0].validation) { + this.notify.error(changeset.errors[0].validation, ENV.notifications); + } else { + this.notify.error(this.tPleaseTryAgain); + }🧰 Tools
🪛 Biome (1.9.4)
[error] 146-146: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/project-settings/integrations/github-project/index.ts (1)
121-123: Consider using a more semantic name for this property.While
hasNoGithubProjectsworks, it uses a double negative which can be confusing. Consider renaming to something likeisGithubProjectsEmptyfor better readability.- get hasNoGithubProjects() { - return this.githubRepos.length === 0; - } + get isGithubProjectsEmpty() { + return this.githubRepos.length === 0; + }tests/integration/components/organization/integrations/service-now-test.js (2)
176-190: Test case missing assertion count.The
assert.expect()call doesn't specify the number of assertions, which is recommended to ensure all expected assertions are actually performed.- assert.expect(); + assert.expect(7); // Update with the actual number of assertions
367-367: Typo in code comment.There's a typo in the comment: "return correc" instead of "return correct".
- // return correc + // return correct response
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/service-now.pngis excluded by!**/*.png
📒 Files selected for processing (84)
app/adapters/servicenow-config.ts(1 hunks)app/components/github-account/index.hbs(0 hunks)app/components/github-account/index.scss(0 hunks)app/components/jira-account/index.hbs(0 hunks)app/components/marketplace/integration-list/index.ts(1 hunks)app/components/organization/integrations/github-account/index.hbs(1 hunks)app/components/organization/integrations/github-account/index.ts(4 hunks)app/components/organization/integrations/index.hbs(1 hunks)app/components/organization/integrations/index.scss(1 hunks)app/components/organization/integrations/index.ts(1 hunks)app/components/organization/integrations/integrated-ui/index.hbs(1 hunks)app/components/organization/integrations/integrated-ui/index.scss(0 hunks)app/components/organization/integrations/integrated-ui/index.ts(1 hunks)app/components/organization/integrations/jira-account/index.hbs(1 hunks)app/components/organization/integrations/jira-account/index.ts(5 hunks)app/components/organization/integrations/service-now/index.hbs(1 hunks)app/components/organization/integrations/service-now/index.ts(1 hunks)app/components/organization/integrations/service-now/validator.ts(1 hunks)app/components/organization/settings-wrapper/index.ts(1 hunks)app/components/organization/settings/index.hbs(0 hunks)app/components/organization/settings/index.ts(1 hunks)app/components/project-settings/general-settings/github-project/index.hbs(0 hunks)app/components/project-settings/general-settings/index.hbs(1 hunks)app/components/project-settings/general-settings/jira-project/index.hbs(0 hunks)app/components/project-settings/header/index.ts(1 hunks)app/components/project-settings/integrations/config-drawer/index.hbs(1 hunks)app/components/project-settings/integrations/config-drawer/index.scss(1 hunks)app/components/project-settings/integrations/config-drawer/index.ts(1 hunks)app/components/project-settings/integrations/github-project/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/index.ts(9 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.ts(1 hunks)app/components/project-settings/integrations/index.hbs(1 hunks)app/components/project-settings/integrations/index.scss(1 hunks)app/components/project-settings/integrations/index.ts(1 hunks)app/components/project-settings/integrations/integrated-header/index.hbs(1 hunks)app/components/project-settings/integrations/integrated-header/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/index.scss(1 hunks)app/components/project-settings/integrations/jira-project/index.ts(10 hunks)app/components/project-settings/integrations/jira-project/no-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/no-project/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.scss(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.ts(1 hunks)app/components/project-settings/integrations/loading-ui/index.hbs(1 hunks)app/components/project-settings/integrations/loading-ui/index.ts(1 hunks)app/components/project-settings/integrations/service-now/index.hbs(1 hunks)app/components/project-settings/integrations/service-now/index.scss(1 hunks)app/components/project-settings/integrations/service-now/index.ts(1 hunks)app/components/project-settings/integrations/service-now/selected-threshold/index.hbs(1 hunks)app/components/project-settings/integrations/service-now/selected-threshold/index.scss(1 hunks)app/components/project-settings/integrations/service-now/selected-threshold/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/service-account.ts(1 hunks)app/controllers/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/enums.ts(1 hunks)app/models/servicenow-config.ts(1 hunks)app/router.ts(3 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/routes/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/templates/authenticated/dashboard/organization-settings/integrations.hbs(1 hunks)app/templates/authenticated/dashboard/project/settings/integrations.hbs(1 hunks)config/environment.js(2 hunks)mirage/factories/github-integration.ts(0 hunks)mirage/factories/jira-integration.ts(0 hunks)mirage/factories/jira-repo.ts(1 hunks)mirage/factories/servicenow-config.ts(1 hunks)mirage/scenarios/default.js(0 hunks)tests/integration/components/github-account-test.js(0 hunks)tests/integration/components/jira-account-test.js(0 hunks)tests/integration/components/organization/integrations/github-account-test.js(1 hunks)tests/integration/components/organization/integrations/jira-account-test.js(1 hunks)tests/integration/components/organization/integrations/service-now-test.js(1 hunks)tests/integration/components/project-settings/general-settings/github-project-test.js(0 hunks)tests/integration/components/project-settings/general-settings/jira-project-test.js(0 hunks)tests/integration/components/project-settings/integrations/github-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/jira-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/service-now-test.js(1 hunks)translations/en.json(8 hunks)translations/ja.json(8 hunks)types/irene/index.d.ts(1 hunks)
💤 Files with no reviewable changes (14)
- mirage/factories/jira-integration.ts
- mirage/factories/github-integration.ts
- mirage/scenarios/default.js
- app/components/organization/integrations/integrated-ui/index.scss
- app/components/github-account/index.hbs
- app/components/organization/settings/index.hbs
- app/components/project-settings/general-settings/jira-project/index.hbs
- app/components/github-account/index.scss
- tests/integration/components/project-settings/general-settings/github-project-test.js
- app/components/project-settings/general-settings/github-project/index.hbs
- tests/integration/components/jira-account-test.js
- app/components/jira-account/index.hbs
- tests/integration/components/github-account-test.js
- tests/integration/components/project-settings/general-settings/jira-project-test.js
🧰 Additional context used
🪛 Biome (1.9.4)
tests/integration/components/project-settings/integrations/service-now-test.js
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
tests/integration/components/project-settings/integrations/jira-project-test.js
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
app/components/organization/integrations/service-now/index.ts
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/integration/components/project-settings/integrations/github-project-test.js
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Run Integration Tests - Part 8
- GitHub Check: Run Integration Tests - Part 7
- GitHub Check: Run Integration Tests - Part 6
- GitHub Check: Run Integration Tests - Part 5
- GitHub Check: Run Integration Tests - Part 4
- GitHub Check: Run Integration Tests - Part 3
- GitHub Check: Run Integration Tests - Part 2
- GitHub Check: Run Integration Tests - Part 1
- GitHub Check: Run Acceptance Tests
- GitHub Check: Run Linting
- GitHub Check: Run E2E Tests (5)
- GitHub Check: Run E2E Tests (4)
- GitHub Check: Run E2E Tests (3)
- GitHub Check: Run E2E Tests (2)
- GitHub Check: Run E2E Tests (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (123)
app/components/project-settings/general-settings/index.hbs (1)
4-4: Padding adjustment improves UI spacing consistency.The change from
pt-3(padding-top) topy-3(padding-top and padding-bottom) provides more consistent vertical spacing throughout the component.tests/integration/components/organization/integrations/jira-account-test.js (3)
382-402: Address TODO regarding timing-dependent test assertions.The commented-out assertions with the TODO note highlight an issue with timing-dependent tests. The integration works but
checkJiratakes time, causing the assertions to fail.Consider implementing one of the following approaches to handle this timing issue:
- Use a waitUntil helper to wait for specific conditions:
await waitUntil(() => find('[data-test-jiraAccount-desc]') === null);
- Or use settled() to wait for all async operations:
await fillIn('[data-test-jiraAccount-hostInput]', 'https://appknox.atlassian.net/'); await fillIn('[data-test-jiraAccount-usernameInput]', 'appknox'); await fillIn('[data-test-jiraAccount-apiKeyInput]', '1234abcd'); await click('[data-test-jiraAccount-integrateBtn]'); await settled(); // Then run your assertions
161-264: Excellent test coverage for disconnection scenarios.The test properly verifies both success and failure cases when disconnecting a Jira account, including checking for proper UI state changes and notifications.
289-404: Comprehensive test cases for integration with proper error handling.The test cases thoroughly verify multiple integration scenarios including validation failures with specific error messages for different error conditions.
app/components/organization/integrations/index.scss (1)
1-6: Well-structured container styling using CSS variables.The use of CSS variables for theming (background color) is good practice as it allows for easy theme changes. The styling provides consistent spacing with appropriate margin and padding values.
mirage/factories/servicenow-config.ts (1)
4-6:❓ Verification inconclusive
Factory implementation properly models ServiceNow risk threshold.
The factory generates random values between 1-4 for risk_threshold, which appears to correctly model the ServiceNow configuration for testing purposes.
One thing to verify:
🏁 Script executed:
#!/bin/bash # Check if the risk_threshold values match the actual model definition rg -A 5 "riskThreshold|risk_threshold" --type typescript app/models/Length of output: 105
Action: Verify risk_threshold consistency with model definitions manually.
The factory in
mirage/factories/servicenow-config.tscorrectly generates a randomrisk_thresholdbetween 1–4 for the ServiceNow configuration. However, our initial automated check returned an error due to an unsupported file type filter. To properly verify that the factory’s values match the model definition inapp/models/, please re-run the search using a file-extension filter (e.g., for.tsfiles):rg -A 5 "riskThreshold|risk_threshold" -g "*.ts" app/models/If this command still yields no output, a manual review of the model definition is recommended to ensure that the configuration values are consistent.
app/components/project-settings/integrations/index.scss (1)
1-11: Clean and well-structured stylingThe CSS for the project settings integrations is well organized with proper nesting. Using CSS variables for colors is a good practice for maintainability and theming support.
app/components/project-settings/integrations/jira-project/selected-project/index.scss (1)
6-15: Vertical divider implementation looks goodThe implementation of the vertical divider with fixed dimensions and margin spacing is clean and follows good practices. Using CSS variables for colors improves maintainability.
mirage/factories/jira-repo.ts (1)
1-8: Factory implementation follows best practicesThe Mirage factory effectively generates mock data for Jira repository integration testing. Using Faker for generating randomized test data is appropriate, and the risk threshold values correctly align with the application's risk levels.
app/enums.ts (1)
313-316: ServiceNow table selection enum follows established patternsThe new
SERVICE_NOW_TABLE_SELECTIONenum follows the established pattern in the file and provides the necessary constants for ServiceNow integration.app/routes/authenticated/dashboard/project/settings/integrations.ts (1)
1-11: Good route implementation with proper inheritance and model hookThe route correctly extends both ScrollToTop and AkBreadcrumbsRoute, following Ember's composition pattern. The model hook properly retrieves the project model from the parent route.
types/irene/index.d.ts (1)
154-159: Good addition of source property to Error interfaceThe addition of the
sourceproperty with an optionalpointerfield follows JSON:API error specification standards, which will help with more precise error localization in forms and API responses.app/components/project-settings/header/index.ts (1)
45-49: Integration tab item added appropriatelyThe new tab for integration settings is properly implemented and positioned logically within the existing tab structure. This addition aligns well with the PR's objective of adding ServiceNow integration functionality.
app/templates/authenticated/dashboard/project/settings/integrations.hbs (1)
1-5: Template implementation looks goodThe template structure is clean and follows the established pattern for settings pages by including the appropriate header and integrations components with the project model.
app/components/project-settings/integrations/loading-ui/index.hbs (1)
1-9: Well-structured loading UI componentThe loading UI component is well-designed with appropriate skeleton placeholders and spacing to provide visual feedback during loading states. The nested structure with different dimensions for each skeleton element will create a natural-looking loading state that matches the content layout.
app/components/organization/integrations/integrated-ui/index.hbs (1)
1-51: Well-structured reusable integration UI component.This component is well-designed with:
- Appropriate loading states with skeleton loaders
- Clear data test attributes for UI testing
- Consistent layout using AkStack components
- Proper internationalization for button text
The generic nature of this component will facilitate reuse across different integration types (GitHub, JIRA, ServiceNow).
app/components/project-settings/integrations/jira-project/no-project/index.hbs (1)
1-23: Well-structured component with clear conditional messaging.This component handles the "no JIRA project" state effectively with:
- Conditional messaging depending on whether it's a reconnect scenario
- Clear navigation path to the organization settings integrations page
- Proper internationalization for all text elements
- Appropriate test selectors for UI testing
app/controllers/authenticated/dashboard/organization-settings/index.ts (2)
5-5: Good addition of explicit type import.Adding this import improves code clarity and type safety.
11-11: Improved type safety with explicit model declaration.Adding the explicit type declaration for the model property enhances type safety, provides better IDE support, and makes the code more maintainable.
app/controllers/authenticated/dashboard/organization-settings/service-account.ts (1)
21-23: Clean addition of sibling routes for improved navigationThe addition of the
siblingRoutesproperty enhances the breadcrumbs navigation structure, creating a relationship between the service account page and the integrations page. This improvement provides users with better navigation context within the organization settings section.app/components/project-settings/integrations/loading-ui/index.ts (1)
1-9: Well-structured loading component with proper type definitionsThe component is correctly defined with Glint registry support, which provides proper TypeScript type checking for templates. The loading UI component follows a minimalist approach with no additional functionality, which is appropriate for a loading indicator.
app/components/organization/integrations/index.ts (1)
1-16: Well-structured component with proper type definitionsThe component is correctly defined with TypeScript interfaces and Glint registry support. The component accepts a model argument of type
OrganizationSettingsIntegrationsRouteModelwhich maintains type safety throughout the application.A few observations:
- The component class is empty, suggesting that all functionality is provided by the template
- The component follows the pattern of other integration components
- Type definitions are properly set up for Glint integration
app/components/marketplace/integration-list/index.ts (4)
14-14: Updated route to point to more specific integration pageThe GitHub integration link now correctly points to the dedicated integrations page rather than the general organization settings.
20-21: Updated route to point to more specific integration pageThe Jira integration link now correctly points to the dedicated integrations page rather than the general organization settings.
22-28: ServiceNow integration properly added to the marketplace listThe ServiceNow integration entry is structured consistently with the other integrations, including:
- Proper translation keys for title and description
- Reference to the ServiceNow logo image
- Correct link to the organization settings integrations page
This addition completes the PR's goal of adding ServiceNow alongside existing GitHub and JIRA integrations.
23-26:✅ Verification successful
Verify that the ServiceNow translation keys and image exist
Ensure that the translation keys (
serviceNow.title) and the image file (../images/service-now.png) are properly defined and available in the codebase.
🏁 Script executed:
#!/bin/bash # Check if ServiceNow translations exist echo "Checking for ServiceNow translation keys..." rg -A 1 "serviceNow" translations/ # Check if service-now.png exists echo "Checking for ServiceNow image file..." fd "service-now.png" public/Length of output: 1332
ServiceNow Resources Verified – No Action Needed
- Verified that the translation key
serviceNow.titleexists in bothtranslations/en.jsonandtranslations/ja.json.- Confirmed that the image file at
public/images/service-now.pngis present.app/models/servicenow-config.ts (2)
1-10: Well-structured ServiceNow config modelThe model implementation follows Ember Data best practices with proper TypeScript type declarations. The relationship to the project model is correctly defined as an async belongs-to relationship with no inverse.
12-16: Good type registry declarationProper module augmentation for the ModelRegistry ensures type safety when using this model throughout the application.
config/environment.js (2)
275-277: ServiceNow endpoint properly definedThe ServiceNow endpoint has been correctly added to the endpoints object, matching the pattern of other integration endpoints like GitHub and JIRA.
475-479: CSB tracking object properly configuredThe CSB tracking object for ServiceNow integration follows the same structure as other integration tracking objects like GitHub and JIRA integrations.
app/components/project-settings/integrations/service-now/selected-threshold/index.hbs (1)
1-16: Well-structured threshold display componentThe component is well-organized with proper spacing, typography, and test attributes. The risk threshold is correctly bound to the ServiceNow configuration model.
Good use of internationalization with the translation helper for the "threshold" text, allowing for localization support.
app/components/organization/settings/index.ts (2)
2-4: Import simplification and type updateThe simplified service import syntax and updated type reference to OrganizationSettingsRouteModel improve code readability and maintainability.
8-9: Updated model type referenceThe model argument type now correctly references the OrganizationSettingsRouteModel from the route, maintaining proper type safety.
app/routes/authenticated/dashboard/organization-settings/integrations.ts (1)
1-61:❓ Verification inconclusive
Strong implementation of the organization settings integrations route
This implementation provides a solid foundation for the organization-level integrations route. The code follows the Ember.js conventions, extends the appropriate base class for breadcrumb support, and properly handles API errors.
A few observations:
Even though this PR seems to be about ServiceNow integration, this route is only fetching GitHub integration data (line 37). Consider revising to support multiple integration types or adding comments to explain this design decision.
The error handling approach for the 400 status code to set the reconnect flag is a good pattern.
The type definitions for
IntegratedUserandOrganizationSettingsIntegrationsRouteModelare clear and well-structured.
🏁 Script executed:
#!/bin/bash # Let's see if there are other integration endpoints planned for ServiceNow rg -A 5 $'/api/organizations/.*?/servicenow'Length of output: 45
Clarify Intended Integration Endpoints
The current implementation adheres well to Ember.js conventions, extends the proper base class for breadcrumb support, and handles API errors cleanly. The type definitions and error-handling pattern are strong. However, the following point needs further attention:
- Integration Mismatch: Although the PR title suggests a ServiceNow integration, the route currently retrieves only GitHub integration data (see line 37). A search for ServiceNow-related endpoints (e.g.,
/api/organizations/<id>/servicenow) did not yield any results. Please verify whether this behavior is intentional. If the design is to support multiple integrations in the future, consider adding comments or a TODO to clarify that GitHub is the only active integration endpoint at this stage.app/components/project-settings/integrations/integrated-header/index.hbs (1)
1-52:✅ Verification successful
Well-structured component with clear conditional rendering
The integrated header component is well-implemented with proper use of conditional rendering and component composition. The header actions menu implementation is particularly elegant.
I notice this template refers to several properties and methods (
handleHeaderActionsOpen,handleHeaderActionsClose,headActionsRef,headerActionsList) that should be defined in the component class file. Let's verify that these methods and properties are properly implemented:
🏁 Script executed:
#!/bin/bash # Check for the component class implementation rg -A 15 "integrated-header/index\.(ts|js)"Length of output: 45
Could you please run the above script on your end to verify whether these properties and methods are defined in the component's class file (or elsewhere in the repository)? This will help confirm if the integrated header properly implements the functionalities referenced in the template.
If the definitions are found in a different file or location, please ensure that the file naming conventions are consistent or update the template references accordingly. If they are missing, you might need to add them to the component class file.
🏁 Script executed:
#!/bin/bash # Search across the repository for the implementation references from the integrated header component. # Specifically, we're looking for the definitions of handleHeaderActionsOpen, handleHeaderActionsClose, headActionsRef, and headerActionsList. rg -n -e "handleHeaderActionsOpen" -e "handleHeaderActionsClose" -e "headActionsRef" -e "headerActionsList" --type js --type tsLength of output: 1049
Integrated header component verified: All related methods and properties are correctly implemented.
The component’s template and its backing class file are in sync. Specifically, the properties and methods—handleHeaderActionsOpen, handleHeaderActionsClose, headActionsRef, and headerActionsList—are properly defined in
app/components/project-settings/integrations/integrated-header/index.ts(e.g., see lines 20, 22, 47, 52). No further changes are necessary.app/router.ts (3)
106-106: Integration route added to project settingsThis adds the integrations route under project settings, consistent with adding ServiceNow integration support.
181-181: Integration route added to dashboard project settingsThis adds the integrations route under dashboard project settings, maintaining consistency across the application.
285-285: Integration route added to organization settingsThis adds the integrations route under organization settings, completing the integration routes across all relevant sections.
app/components/organization/integrations/index.hbs (1)
1-16: Well-structured integration components layout!The template provides a clean organization of the three integration components (Jira, GitHub, and ServiceNow) with appropriate dividers between them. The component structure is consistent and each receives the necessary props from the model.
app/components/organization/integrations/integrated-ui/index.ts (3)
3-13: Interface definition looks appropriateThe interface properly defines all the necessary properties for an integrated UI component, including image information, host information, username, and callback functions.
15-15: Component implementation is minimal but sufficientThe component extends the base Component class with the defined interface type. Since there's no custom logic needed, the empty class implementation is appropriate.
17-21: Glint registry declaration is correctly implementedThe module declaration properly registers the component for type checking in templates.
app/components/project-settings/integrations/github-project/selected-repo/index.ts (3)
4-8: Interface definition is properly structuredThe interface correctly defines that this component expects a GitHub repository model as its argument.
10-10: Component implementation is appropriately definedSimilar to other components, the class extends the base Component with the proper interface type.
12-16: Type registry entry is correctly implementedThe Glint registry properly maps the component name to its type.
app/components/project-settings/integrations/index.hbs (1)
1-23: Good integration structure with proper component organization.The layout nicely organizes all three integration components (Jira, GitHub, and ServiceNow) with proper spacing and separators between them.
app/components/organization/integrations/github-account/index.hbs (1)
1-47: Well-structured conditional rendering for GitHub integration.The template properly handles different states (integrated, not integrated, reconnect) with appropriate user messaging and actions for each case. Good use of the IntegratedUi component for the connected state and clear connection workflows.
app/components/project-settings/integrations/jira-project/selected-project/index.ts (1)
1-16: Clean component implementation that follows best practicesThe component is well structured with a clear interface definition for its arguments and proper type registration for Glint. The design follows the Ember Octane pattern using Glimmer components.
app/components/project-settings/integrations/jira-project/no-project/index.ts (1)
1-15: Well-structured component with appropriate typingThis component correctly implements the pattern for handling the "no project" state. The boolean
reconnectargument suggests this component might display reconnection options, which aligns with its purpose.app/components/project-settings/integrations/github-project/selected-repo/index.hbs (1)
1-41: Well-structured UI with proper accessibility considerationsThe template has a clean layout with appropriate spacing and typography. Good accessibility practices are followed with proper attributes such as
rel='noopener noreferrer'for external links. The data-test attributes also facilitate testing.app/controllers/authenticated/dashboard/project/settings/integrations.ts (2)
13-32: Well-implemented breadcrumbs structure with proper internationalizationThe breadcrumbs implementation provides a clear navigation hierarchy and uses internationalization appropriately. This will enhance the user experience by providing context within the application.
17-17: Ensure model ID is never undefinedThe optional chaining on
this.model?.idsuggests the model might be undefined, but this array is used in a route definition where undefined values could cause issues.- models: [this.model?.id], + models: [this.model.id],Verify that the model is always defined when this getter is called, or add a fallback value if it might be undefined.
app/components/project-settings/integrations/config-drawer/index.hbs (1)
1-80: Well-structured drawer component with appropriate separation of concerns.The component has a clean organization with header, content, and footer sections. The use of data-test attributes is good for testing, and the component properly handles loading states for the save button.
app/components/project-settings/integrations/config-drawer/index.ts (1)
1-21: Clean component implementation with good type definitions.The component has a well-defined interface and proper type registrations. The separation of concerns between the TypeScript class and the Handlebars template is appropriate.
app/components/organization/integrations/jira-account/index.hbs (1)
1-79: Well-structured conditional rendering with appropriate form validation.The component handles different states well (integrated vs. non-integrated) and uses proper form validation through the changeset pattern. The password field is correctly handled as a password type, and appropriate data-test attributes are used for testing.
tests/integration/components/organization/integrations/github-account-test.js (1)
1-261: Looks great!All tests appear consistent, logically sound, and provide ample coverage for both integrated and non-integrated scenarios. Overall, well-structured and clear.
app/components/project-settings/integrations/integrated-header/index.ts (7)
1-6: Well-structured component importsThe file has a clean organization of imports with appropriate type definitions for the IntlService.
7-15: Good use of TypeScript interface for component argumentsThe interface defines clear contract with required and optional properties, enforcing that consumers must provide the necessary callback functions.
17-21: Proper declaration of services and tracked propertiesThe component correctly uses dependency injection for the intl service and tracks the reference to the HTML element for header actions.
22-34: Well-implemented internationalized action listThe headerActionsList getter returns a properly structured array of actions with translated labels.
36-44: Effective action handling with proper cleanupBoth action handlers first close the header actions menu before executing the respective callback, ensuring a clean UI state.
46-54: Clean implementation of open/close handlersThe component manages the dropdown state cleanly by setting and clearing the reference to the target HTML element.
57-61: Proper Glint type registrationThe component is correctly registered with Glint for type checking, enhancing development experience.
translations/en.json (4)
201-204: Consistent confirmation messages for ServiceNow integrationThe added confirmation messages for ServiceNow follow the same pattern as the existing GitHub and JIRA confirmation messages.
1045-1045: Consistent ServiceNow threshold messageThe message for selecting ServiceNow risk threshold follows the same pattern as other integration messages.
1455-1464: Complete set of ServiceNow-specific translationsAll necessary translations for the ServiceNow integration have been added following the same structure as other integrations.
1465-1467: Consistent integration messages for ServiceNowIntegration status and description messages follow the same pattern as GitHub and JIRA integrations.
app/routes/authenticated/dashboard/organization-settings/index.ts (2)
2-15: Improved type definitions with proper importsThe route now uses proper type definitions for models and services, enhancing type safety.
21-31: Simplified model hook with proper typingThe model hook has been simplified to return only the essential data with appropriate TypeScript typing, making the code more maintainable.
app/controllers/authenticated/dashboard/organization-settings/integrations.ts (3)
1-7: Clean controller imports with appropriate type definitionsThe controller imports only what's needed with proper type annotations.
8-12: Well-declared controller with appropriate service injectionThe controller correctly declares the intl service and model type.
13-29: Comprehensive breadcrumb navigation setupThe breadcrumbs computed property provides a complete navigation structure with proper internationalization, parent-child relationships, and sibling routes.
app/components/organization/integrations/service-now/index.hbs (5)
1-7: Solid heading usage.
These lines effectively introduce the ServiceNow Integration title with the correct variant and data-test attribute. No issues found.
21-27: Description text is well-structured.
The typography and data-test attributes are clean and follow your naming conventions. No concerns here.
63-82: ServiceNow table selection logic looks consistent.
TheAkSelectusage and data-test are well-structured. No issues spotted.
85-95: Integration button is straightforward.
Good use of a loading state and performing the integration task on click. Implementation seems clean.
97-103: Confirmation box setup is correct.
Links properly to the showRevokeServiceNowConfirmBox property. No concerns.app/components/organization/integrations/service-now/index.ts (2)
52-91: Constructor and property definitions look solid.
The changeset initialization with custom validation is appropriate. No major issues spotted.
141-164: checkServiceNowIntegration task is well-structured.
The code properly handles 404 scenarios to mark no integration exists. Good job managing the isServiceNowIntegrated flag and selected table item.app/components/project-settings/integrations/github-project/index.hbs (2)
1-52: Well-structured layout with good state handling.The GitHub project integration template has a well-organized structure that handles different states effectively:
- Loading state when fetching repositories
- Display of selected repository
- CTA for project selection
- Fallback for no integration or repositories
The consistent use of data-test attributes throughout the template will make testing more robust.
116-122: Appropriate confirmation box implementation.The confirmation box for deleting GitHub projects is properly implemented with:
- Descriptive message via translation
- Disabled state during deletion process
- Proper delegation for handling confirmation actions
app/components/project-settings/integrations/service-now/index.hbs (3)
6-64: Comprehensive state handling for ServiceNow integration.The template handles multiple states effectively:
- Loading state during config fetching
- Displaying selected threshold when available
- Access control with appropriate error messaging
- No integration state with a helpful link to settings
- CTA for threshold selection
The conditional logic is well-structured and handles all possible states.
66-106: Well-implemented configuration drawer.The configuration drawer follows the same pattern as other integration components, providing a consistent user experience. It correctly allows threshold selection with appropriate data test attributes for testing.
108-114: Confirmation box implementation is correct.The delete confirmation modal is properly implemented with the necessary properties for activation, description, delegation, and disabling during deletion.
app/components/organization/integrations/jira-account/index.ts (5)
19-28: Good type definitions for JIRA integration.The addition of
JIRAIntegrationFieldstype and the update toChangesetBufferPropsimprove type safety and make the component's interface more explicit, which is a good practice.
35-41: Improved component naming for better organization.Renaming the interface from
JiraAccountSignaturetoOrganizationIntegrationsJiraAccountSignaturefollows a more structured naming convention that better reflects the component's position in the component hierarchy.
69-72: Updated constructor signature for type consistency.The constructor has been properly updated to use the renamed interface, maintaining type safety.
100-106: Useful computed properties for state management.The new computed properties
isLoadingJIRAIntegrationInfoandshowIntegratedOrLoadingUImake template logic more readable by abstracting state checks.
198-198: Updated registry entry for component.The Glint registry entry has been correctly updated to match the new component name, ensuring type checking works properly in templates.
app/components/project-settings/integrations/jira-project/index.hbs (3)
1-64: Well-structured main component with comprehensive state handling.The JIRA project integration template follows the same pattern as the other integration templates, providing:
- Clear layout structure
- Proper header with actions
- Comprehensive state handling (loading, selected project, no project, no access, no integration)
- Appropriate UI elements for each state
The nested conditional structure is slightly different from other integration templates but achieves the same functionality.
66-138: Well-designed configuration drawer with enhanced user guidance.The configuration drawer includes:
- Repository and threshold selection with appropriate labeling
- Good use of data-test attributes
- A helpful note section with HTML-safe translation
- Visual separation using
AkDividerThis implementation provides better user guidance compared to other integration drawers.
140-146: Confirmation box implementation is correct.The delete confirmation box is properly implemented with the necessary properties for activation, description, delegation, and disabling during deletion.
app/components/project-settings/integrations/github-project/index.ts (11)
18-19: Interface name update aligns with new integrations structure.The interface rename from
ProjectSettingsGeneralSettingsGithubProjectSignaturetoProjectSettingsIntegrationsGithubProjectSignaturebetter reflects its placement within the integrations context rather than general settings.
24-24: Component class name update matches interface changes.The class rename from
ProjectSettingsGeneralSettingsGithubProjectComponenttoProjectSettingsIntegrationsGithubProjectComponentmaintains consistency with the interface rename.
35-36: Add state tracking for GitHub integration absence.The
noIntegrationtracked property helps properly handle UI state when no GitHub integration exists at the organization level.
74-76: Useful UI visibility control based on GitHub repo state.The
showHeaderActionsgetter provides a cleaner way to conditionally render header actions based on the current integration state.
78-92: Comprehensive header subtext logic for different integration states.The
headerSubTextgetter effectively handles multiple states:
- When reconnection is needed or integration is missing
- When a GitHub account is integrated
- When GitHub projects exist but none is selected
94-104: Clear subtext handling for non-integration scenarios.The
noIntegrationOrNoRepoSubtextgetter provides appropriate messaging for reconnect, no integration, or no project scenarios.
137-139: Clean conditional logic for CTA visibility.The
showSelectPrjCTAgetter properly combines conditions to determine when to show the select project call-to-action.
193-207: Improved error handling with explicit condition checks.The code now handles different GitHub integration error states more explicitly, with clearer separation between "not integrated" and "integration failed" conditions.
235-247: Enhanced error handling for fetching GitHub repos.The error handling for
fetchGithubReposnow properly distinguishes between integration failure and missing integration.
305-311: Setting multiple related state properties.When handling "Github not integrated" errors in the
selectProjecttask, the code properly updates multiple state properties to ensure consistent UI rendering.
339-339: Updated component registry name.The registry name has been updated to match the new component naming structure, maintaining consistency throughout the application.
tests/integration/components/organization/integrations/service-now-test.js (6)
1-14: Well-structured imports for testing ServiceNow integration.The imports include all necessary testing utilities, including Mirage for API mocking, which is essential for integration testing.
15-19: Using class mapping for maintainable selectors.Centralizing CSS class selectors at the top of the file makes tests more maintainable if the underlying styles change.
21-38: Service stubs for isolated testing.The
OrganizationMeStubandNotificationsStubservices properly isolate the component from the actual services, allowing controlled testing of component behavior.
40-104: Reusable assertion helper for non-integrated UI state.The
assertNonIntegratedUIfunction encapsulates all assertions for the non-integrated state, making tests more readable and maintainable.
106-159: Reusable assertion helper for integrated UI state.Similar to the non-integrated helper, this function encapsulates all assertions for the integrated state.
344-350:❓ Verification inconclusive
Duplicate label mapped to same value.
The
snTableItemsarray contains two items with different labels but the same value from the enum. Verify if this is intentional or a mistake.
🏁 Script executed:
#!/bin/bash # Check if these service now tables are actually different or should have different enum values # Look for the enum definition rg -A 10 "SERVICE_NOW_TABLE_SELECTION" | grep -A 10 "="Length of output: 1447
Action Required: Confirm Duplicate Enum Value Mapping
The
snTableItemsarray currently includes two entries with different labels:
'sn_vul_app_vulnerable_item''sn_vul_vulnerable_item'Both are mapped to the same enum value:
ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_APP_VULNERABLE_ITEM.Please verify if this duplication is intentional—for instance, if the two labels are meant to represent the same underlying table selection with different display contexts—or if one of these entries should be updated to use a distinct enum value. If it’s a mistake, consider adjusting the mapping or cleaning up the data structure accordingly.
app/components/organization/integrations/github-account/index.ts (5)
28-34: Interface rename for better organization structure.Renaming the interface to
OrganizationIntegrationsGithubAccountSignaturebetter reflects its place in the component hierarchy.
36-36: Component rename matches interface changes.The class rename to
OrganizationIntegrationsGithubAccountComponentmaintains consistency with the interface.
48-51: Constructor updated to use the new interface type.The constructor parameter has been properly updated to match the renamed interface.
60-73: Added methods for UI state management.New action methods to handle GitHub integration revocation UI state:
openRevokeGithubConfirmBoxcloseRevokeGithubConfirmBoxconfirmCallbackThese methods provide a cleaner interface for managing the confirmation box visibility.
120-120: Updated registry name for consistency.The registry entry has been updated to use the namespace format that matches the component's new name.
app/components/project-settings/integrations/jira-project/index.ts (9)
1-4: Updated import style frominject as servicetoservice.The code is using the newer
servicedecorator import rather than the olderinject as servicepattern, which is good practice.
29-30: Interface rename for better organization structure.Renaming the interface to
ProjectSettingsIntegrationsJiraProjectSignaturealigns with the integrations refactoring pattern seen across the application.
35-35: Component class name updated for consistency.The class rename to
ProjectSettingsIntegrationsJiraProjectComponentmaintains consistency with the rest of the integration components.
106-126: Added computed properties improve component readability.The new getters for:
showHeaderActionsheaderSubTextisLoadingJiraInfoThese provide cleaner access to derived state and help with UI rendering decisions.
177-193: Improved error handling with better extraction of error details.Extracting the error detail into a variable makes the error handling code more readable and less prone to repetition.
211-227: Simplified error handling flow with early returns.The error handling in
fetchJIRAProjectsis now more direct and uses early returns for specific error conditions, making the code easier to follow.
273-274: Removed unnecessary type assertion in save method call.The code now directly awaits the
jiraProject.save()result without type assertions, which simplifies the code.
287-288: Destructuring error properties improves code readability.Using destructuring for error properties makes the error handling code cleaner and more maintainable.
- const error = err as AdapterError; - const { source, detail: errorDetail } = error?.errors?.[0] || {}; + const { errors } = err as AdapterError; + const { source, detail: errorDetail } = errors?.[0] || {};
315-315: Updated registry name for consistency.The registry entry has been updated to use the namespace format that matches the component's new name.
app/templates/authenticated/dashboard/organization-settings/integrations.hbs
Show resolved
Hide resolved
app/components/project-settings/integrations/service-now/selected-threshold/index.ts
Outdated
Show resolved
Hide resolved
app/components/project-settings/integrations/service-now/selected-threshold/index.ts
Outdated
Show resolved
Hide resolved
app/components/project-settings/integrations/service-now/selected-threshold/index.ts
Outdated
Show resolved
Hide resolved
app/components/project-settings/integrations/service-now/index.scss
Outdated
Show resolved
Hide resolved
app/components/project-settings/integrations/service-now/index.scss
Outdated
Show resolved
Hide resolved
app/components/project-settings/integrations/service-now/index.scss
Outdated
Show resolved
Hide resolved
app/components/project-settings/integrations/jira-project/index.scss
Outdated
Show resolved
Hide resolved
tests/integration/components/organization/integrations/service-now-test.js
Outdated
Show resolved
Hide resolved
Irene
|
|||||||||||||||||||||||||||||||||||||
| Project |
Irene
|
| Branch Review |
PD-1715-service-now-integration-ui-changes
|
| Run status |
|
| Run duration | 07m 26s |
| Commit |
|
| Committer | Yibaebi Elliot |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
2
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
30
|
| View all changes introduced in this branch ↗︎ | |
Tests for review
cypress/tests/upload-app.spec.ts • 2 failed tests
| Test | Artifacts | |
|---|---|---|
| Upload App > It successfully uploads an apk file |
Test Replay
Screenshots
|
|
| Upload App > It successfully uploads an ipa file |
Test Replay
Screenshots
|
|
Yibaebi
force-pushed
the
PD-1715-service-now-integration-ui-changes
branch
from
a5b038f to
da5de51
Compare
Yibaebi
force-pushed
the
PD-1715-service-now-integration-ui-changes
branch
from
da5de51 to
4b4ab17
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/adapters/servicenow-config.ts (1)
13-24:⚠️ Potential issueAdd error handling to the updateRecord method.
The updateRecord method doesn't include any error handling, which could lead to unhandled promise rejections.
updateRecord<K extends keyof ModelRegistry>( _: Store, _type: ModelSchema<K>, snapshot: Snapshot<K> ) { const { riskThreshold } = snapshot.record as ServiceNowConfigModel; const url = this._buildURL(); - return this.ajax(url, 'POST', { - data: { risk_threshold: riskThreshold }, - }); + return this.ajax(url, 'POST', { + data: { risk_threshold: riskThreshold }, + }).catch(error => { + console.error('Error updating ServiceNow config:', error); + throw error; + }); }
🧹 Nitpick comments (28)
app/components/organization/integrations/integrated-ui/index.scss (3)
1-7: Outer Container Styling ReviewThe
.bordered-boxclass is well structured overall, consolidating integration styles effectively. The use of CSS variables for border color and border-radius helps maintain consistency. However, consider verifying if the fixedmax-width: 800pxmeets responsive design needs across various devices.
8-16: Logo Container Styling & ReadabilityThe nested
.integration-logo-containerapplies a clear padding and a right border to separate logo content. For improved readability, consider combining the broken border declaration into one line unless dictated by style guidelines. Here’s a suggested diff:- border-right: 1px solid - var(--organization-integrations-integrated-ui-border-color); + border-right: 1px solid var(--organization-integrations-integrated-ui-border-color);Also, ensure the fixed image width of 40px is suitable for the intended logo assets.
18-22: Account Container Styling ConfirmationThe
.integration-account-containersection correctly usesbox-sizing,padding, andwidth: 100%to ensure it occupies the intended space next to the logo container. If future design iterations require enhanced responsiveness, consider adding media queries to adjust the layout on smaller screens.app/controllers/authenticated/dashboard/project/settings/integrations.ts (1)
19-25: Consider including the current route in siblingRoutes.The siblingRoutes array contains related project settings routes but excludes the current route itself. If the intention is to build a navigation menu from these routes, including the current route with active state marking would improve user experience.
siblingRoutes: [ 'authenticated.dashboard.project.settings.index', 'authenticated.dashboard.project.settings.dast-automation', 'authenticated.dashboard.project.settings.analysis', + 'authenticated.dashboard.project.settings.integrations', ],tests/integration/components/project-settings/integrations/github-project-test.js (3)
29-43: Fix typo in function nameThere's a typo in the function name
creatOrgRepo. Consider renaming it tocreateOrgRepofor better readability and consistency.-const creatOrgRepo = (propsOverride = {}) => { +const createOrgRepo = (propsOverride = {}) => {Make sure to update all references to this function throughout the file (e.g., line 117 and 489).
199-200: Use regex literal instead of RegExp constructorUse a regular expression literal instead of the RegExp constructor for better readability and to avoid unnecessary escaping.
- new RegExp('organization/settings/integrations', 'i') + /organization\/settings\/integrations/i🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
117-126: Consider parameterizing GitHub repo creationThe test creates exactly 3 GitHub repos. If this number changes in the future, you'll need to update multiple places (e.g., the array and the assertion). Consider parameterizing this value.
+ const REPO_COUNT = 3; - const orgGithubRepos = [1, 2, 3].map(() => creatOrgRepo()); + const orgGithubRepos = Array(REPO_COUNT).fill().map(() => creatOrgRepo());app/components/project-settings/integrations/github-project/index.ts (2)
121-123: Minor redundancy in hasNoGithubProjects getterThis getter is the exact inverse of the existing
hasGitHubProjectgetter. Consider replacing usages of this getter with!this.hasGitHubProjectto reduce code duplication.- get hasNoGithubProjects() { - return this.githubRepos.length === 0; - }
193-215: Enhanced error handling with granular error checksThe error handling has been improved with more specific checks for different error scenarios. This is good for providing appropriate user feedback based on the specific issue encountered.
Suggestion: Consider extracting these repeated error message checks to a utility method for better maintainability.
+ private isGithubNotIntegratedError(errorDetail: string | undefined): boolean { + return errorDetail === 'Github not integrated'; + } + + private isGithubIntegrationFailedError(errorDetail: string | undefined): boolean { + return errorDetail === 'Github integration failed'; + } setCurrentGithubRepo = task(async () => { try { // ... } catch (e) { const err = e as AdapterError; const errorDetail = err?.errors?.[0]?.detail; // If Repos exist and error is not integrated, then something went wrong with the integration - if (errorDetail === 'Github not integrated' && this.hasGitHubProject) { + if (this.isGithubNotIntegratedError(errorDetail) && this.hasGitHubProject) { this.currentGithubRepo = null; this.reconnect = true; return; } - if (errorDetail === 'Github not integrated') { + if (this.isGithubNotIntegratedError(errorDetail)) { this.currentGithubRepo = null; return; } - if (errorDetail === 'Github integration failed') { + if (this.isGithubIntegrationFailedError(errorDetail)) { this.reconnect = true; return; } // ... } });app/routes/authenticated/dashboard/organization-settings/integrations.ts (1)
41-51: Error handling is incompleteThe try-catch block only handles the 400 error status specifically. Consider adding more comprehensive error handling to provide better feedback for other error scenarios (e.g., network errors, server errors).
try { const data = (await this.ajax.request(url)) as IntegratedUser; if (data) { integratedUser = data; } } catch (err) { if ((err as AjaxError).status === 400) { reconnect = true; + } else { + // Log error or handle other error cases + console.error('Failed to fetch integration data:', err); } }app/components/project-settings/integrations/risk-threshold/index.hbs (1)
1-6: Consider moving inline styles to SCSSWhile the component itself looks well-structured, consider moving the inline styles to your SCSS file instead of using the style helper. This would improve maintainability and separation of concerns.
<AnalysisRisk::Tag @computedRisk={{@computedRisk}} local-class='threshold-risk-label' - {{style width='fit-content' minWidth='65px'}} ...attributes />Then add these styles to your corresponding SCSS file:
.threshold-risk-label { width: fit-content; min-width: 65px; }tests/integration/components/project-settings/integrations/service-now-test.js (1)
114-117: Use a regular expression literal instead of the RegExp constructor.Regular expression literals are more readable and perform better than using the RegExp constructor.
- .hasAttribute( - 'href', - new RegExp('organization/settings/integrations', 'i') - ); + .hasAttribute( + 'href', + /organization\/settings\/integrations/i + );🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
app/components/project-settings/integrations/service-now/index.ts (5)
23-23: Missing type for the NotificationServiceThe
notifyservice declaration is missing its type definition, which could lead to compile-time or runtime errors.- @service('notifications') declare notify: NotificationService; + @service('notifications') declare notify: NotificationService;Import the NotificationService type at the top of the file:
import type NotificationService from 'ember-cli-notifications/services/notifications';
96-103: Consider adding type safety to the threshold parameterAdding type safety to the
thresholdparameter will prevent potential issues when working with this function.- @action selectThreshold(threshold: number) { + @action selectThreshold(threshold: ENUMS.THRESHOLD) { this.selectedThreshold = threshold; }
105-129: Add error handling for network failuresThe current error handling only accounts for specific HTTP status codes and error details, but doesn't handle generic network failures.
fetchServiceNowConfig = task(async () => { try { this.configureSNAdapterNamespace(); const config = await this.store.queryRecord('servicenow-config', {}); this.serviceNowConfig = config; this.selectedThreshold = config.riskThreshold; } catch (err) { const error = err as AdapterError; const { detail: errorDetail, status } = error.errors?.[0] ?? {}; if (status === 404) { this.noAccess = true; return; } if (errorDetail === 'Servicenow not integrated') { this.noIntegration = true; return; } + + // Handle other errors like network failures + this.notify.error(parseError(err)); } });
149-203: Consider simplifying error handling logicThe error handling code could be simplified by grouping related conditions and extracting error handling into a separate method for better readability.
The current implementation has nested conditionals and duplicated logic. Consider refactoring to:
// Example refactored approach: private handleSaveError(error: AdapterError, serviceNowConfig: ServiceNowConfigModel) { const { source, detail: errorDetail } = error?.errors?.[0] || {}; // Handle validation errors if (['/data/attributes/riskThreshold', '/data/attributes/risk_threshold'].includes(String(source?.pointer))) { this.notify.error(this.intl.t('invalidRisk')); return; } // Handle integration errors if (errorDetail === 'ServiceNow not integrated') { this.showEditSNRiskModal = false; this.serviceNowConfig = null; this.noIntegration = true; } this.notify.error(parseError(error)); } // Then in the selectRiskThreshold task: // try { ... } catch (err) { // serviceNowConfig.rollbackAttributes(); // this.handleSaveError(err as AdapterError, serviceNowConfig); // }
149-150: Use more type-safe variable declarationsUsing more specific types in variable declarations improves code clarity and prevents potential errors.
- let serviceNowConfig = this.serviceNowConfig; + let serviceNowConfig: ServiceNowConfigModel | null = this.serviceNowConfig;tests/integration/components/project-settings/integrations/jira-project-test.js (1)
136-138: Use RegExp literal instead of RegExp constructorUsing a RegExp literal is clearer and avoids unnecessary escaping.
- .hasAttribute( - 'href', - new RegExp('organization/settings/integrations', 'i') - ); + .hasAttribute( + 'href', + /organization\/settings\/integrations/i + );🧰 Tools
🪛 Biome (1.9.4)
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
app/components/project-settings/integrations/integrated-header/index.ts (1)
36-44: Consider adding confirmation for delete action.The delete action is triggered immediately upon selection from the dropdown menu. For destructive operations like deletion, it would be safer to add an additional confirmation step to prevent accidental deletions of integrations.
@action handleDeleteAction() { this.handleHeaderActionsClose(); - this.args.deleteAction(); + // Add a confirmation step before executing the delete action + if (confirm(this.intl.t('confirmDelete'))) { + this.args.deleteAction(); + } }Note: I see you're using a ConfirmBox component in other files, which would be a better approach than using the browser's native confirm. Consider implementing that pattern here as well for consistency.
app/components/organization/integrations/service-now/index.hbs (2)
51-61: Enhance password field security.While the password field appropriately uses
type='password'for masking input, consider adding additional security measures:
- The
autocomplete='service-now-password'attribute should beautocomplete='new-password'to prevent browsers from auto-filling the field- Consider adding a toggle to show/hide the password for better UX
<AkTextField @placeholder={{t 'enterPassword'}} @label={{t 'password'}} @labelTypographyVariant='subtitle2' @labelTypographyColor='textPrimary' @type='password' @value={{this.changeset.password}} @error={{this.changeset.error.password}} - autocomplete='service-now-password' + autocomplete='new-password' data-test-orgIntegrations-serviceNow-passwordInput />
85-93: Inform users about integration implications.The button to integrate ServiceNow proceeds immediately without providing information about what will happen after integration. Consider adding explanatory text or a tooltip that explains what permissions will be granted and how the integration works.
app/components/organization/integrations/service-now/index.ts (1)
181-190: Use optional chaining as suggested by static analysis.On line 185, the static analysis tool correctly suggests using optional chaining for safer access to nested properties.
integrateServiceNow = task(async (changeset) => { await changeset.validate(); if (!changeset.isValid) { - if (changeset.errors && changeset.errors[0].validation) { + if (changeset.errors?.[0]?.validation) { this.notify.error(changeset.errors[0].validation, ENV.notifications); } return; }🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/project-settings/integrations/jira-project/index.hbs (1)
22-63: Complex conditional rendering section.The nested conditional rendering (multiple
{{else if}}and{{if}}blocks) creates a complex flow that might be harder to maintain. Consider refactoring this into smaller, more focused components to improve readability and maintainability.For example, you could create separate components for different states (no access, no integration, etc.) and use a single computed property in the TS file to determine which state to render.
app/components/project-settings/integrations/service-now/index.hbs (2)
14-16: Consider handling error states alongside loading.Currently, when
this.fetchServiceNowConfig.isRunningis false, the template checks other conditions (serviceNowConfig,noAccess, etc.). However, if the task fails (e.g., due to a network error) and isn't caught by these conditions, the UI won't display a clear error message. Consider providing an additional branch to handle error states or re-checking the conditions after the task fails.
65-75: Offer a visual prompt or validation for threshold selection.When users click "Select Threshold," they open the drawer to manage risk thresholds. To improve user guidance, consider adding a warning or validation if no threshold is selected. This helps ensure users don’t accidentally close the drawer without making a selection or understanding that a threshold must be chosen.
tests/integration/components/organization/integrations/service-now-test.js (3)
176-190: Refine assertion setup for QUnit.Within this test, you use
assert.expect()without specifying the number of assertions, which is typically expected in QUnit. If you intend to enforce a certain number of assertions, callassert.expect(<number>), or remove the call if it's unneeded to avoid confusion.
248-248: Minor naming consistency.Variable
disconnecBtnis missing the “t” in “disconnect.” For clarity, consider renaming todisconnectBtn.- const disconnecBtn = find( + const disconnectBtn = find(
303-320: Expand negative-path coverage forassertNonIntegratedUI.
assertNonIntegratedUIverifies correct UI elements for a non-integrated state but doesn’t explicitly test for a failed network response scenario. Consider adding or expanding tests to confirm that the UI and notifications handle error states (e.g., network or server errors) appropriately beyond returning 404.
🛑 Comments failed to post (3)
app/routes/authenticated/dashboard/organization-settings/integrations.ts (1)
12-19: 🛠️ Refactor suggestion
Type definition inconsistent with PR purpose
The
IntegratedUsertype appears to be GitHub-specific (with fields likelogin,html_url, andavatar_url), but this PR is about ServiceNow integration. Consider using a more generic interface or creating specific types for each integration platform.-type IntegratedUser = { +type GitHubIntegratedUser = { avatar_url: string; created_on: string; html_url: string; login: string; name: string | null; updated_on: string; }; + +// Add ServiceNow specific type if needed +type ServiceNowIntegratedUser = { + // ServiceNow specific fields +}; + +type IntegratedUser = GitHubIntegratedUser | ServiceNowIntegratedUser;📝 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.type GitHubIntegratedUser = { avatar_url: string; created_on: string; html_url: string; login: string; name: string | null; updated_on: string; }; // Add ServiceNow specific type if needed type ServiceNowIntegratedUser = { // ServiceNow specific fields }; type IntegratedUser = GitHubIntegratedUser | ServiceNowIntegratedUser;app/components/organization/integrations/service-now/index.ts (1)
93-104:
⚠️ Potential issueFix duplicate ServiceNow table value assignment.
Both table items in the
snTableItemsgetter have the same value (ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_APP_VULNERABLE_ITEM), which appears to be a bug. The second option should likely have a different value.get snTableItems() { return [ { label: 'sn_vul_app_vulnerable_item', value: ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_APP_VULNERABLE_ITEM, }, { label: 'sn_vul_vulnerable_item', - value: ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_APP_VULNERABLE_ITEM, + value: ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_VULNERABLE_ITEM, }, ]; }📝 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.get snTableItems() { return [ { label: 'sn_vul_app_vulnerable_item', value: ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_APP_VULNERABLE_ITEM, }, { label: 'sn_vul_vulnerable_item', value: ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_VULNERABLE_ITEM, }, ]; }app/components/project-settings/integrations/jira-project/index.hbs (1)
94-106: 🛠️ Refactor suggestion
Use dynamic translation in placeholder.
The placeholder attribute uses a quoted translation string:
@placeholder='{{t "selectRepo"}}', which won't be properly translated. Remove the quotes to ensure proper interpolation:<AkSelect @options={{this.jiraProjects}} @selected={{this.selectedRepo}} @onChange={{this.selectRepo}} - @placeholder='{{t "selectRepo"}}' + @placeholder={{t "selectRepo"}} @triggerClass='select-repo-class' {{style width='100%'}} data-test-prjSettings-integrations-jiraProject-configDrawer-repoList as |repo| >The same issue occurs with the threshold selector placeholder on line 115.
📝 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.<AkSelect @options={{this.jiraProjects}} @selected={{this.selectedRepo}} @onChange={{this.selectRepo}} @placeholder={{t "selectRepo"}} @triggerClass='select-repo-class' {{style width='100%'}} data-test-prjSettings-integrations-jiraProject-configDrawer-repoList as |repo| > {{repo.key}}-{{repo.name}} </AkSelect> </AkStack>
Yibaebi
force-pushed
the
PD-1715-service-now-integration-ui-changes
branch
from
4b4ab17 to
6ee6554
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
app/adapters/servicenow-config.ts (1)
13-24:⚠️ Potential issueAdd error handling to the updateRecord method.
The updateRecord method doesn't include any error handling, which could lead to unhandled promise rejections.
updateRecord<K extends keyof ModelRegistry>( _: Store, _type: ModelSchema<K>, snapshot: Snapshot<K> ) { const { riskThreshold } = snapshot.record as ServiceNowConfigModel; const url = this._buildURL(); - return this.ajax(url, 'POST', { - data: { risk_threshold: riskThreshold }, - }); + return this.ajax(url, 'POST', { + data: { risk_threshold: riskThreshold }, + }).catch(error => { + console.error('Error updating ServiceNow config:', error); + throw error; + }); }
🧹 Nitpick comments (15)
app/components/project-settings/integrations/integrated-header/index.ts (2)
22-34: Consider accessibility enhancements for action menu.The action list implementation is clean, but consider adding ARIA attributes and keyboard navigation support for the dropdown menu that will use this list.
46-54: Event handling could be more robust.Consider adding error handling for the event casting in
handleHeaderActionsOpenand potentially debounce the open/close actions to prevent rapid toggling.@action handleHeaderActionsOpen(event: MouseEvent) { - this.headActionsRef = event.currentTarget as HTMLElement; + const target = event.currentTarget; + if (target instanceof HTMLElement) { + this.headActionsRef = target; + } }tests/integration/components/project-settings/integrations/github-project-test.js (1)
198-200: Use regex literal notation instead of RegExp constructor.Using regex literals is preferable to the RegExp constructor as they avoid string escaping issues and are easier to statically analyze.
- .hasAttribute( - 'href', - new RegExp('organization/settings/integrations', 'i') - ); + .hasAttribute( + 'href', + /organization\/settings\/integrations/i + );🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
tests/integration/components/organization/integrations/service-now-test.js (3)
177-178: Missing assertion count in test expectations.The
assert.expect()call is missing the number of expected assertions, which makes it harder to ensure all the intended assertions are executed.- assert.expect(); + assert.expect(10);Note: The exact number should reflect the actual count of assertions in this test.
193-194: Missing assertion count in integrated test.Similar to the previous test, this test also has an empty
assert.expect()call.- assert.expect(); + assert.expect(9);Note: The exact number should reflect the actual count of assertions in this test.
198-198: Hardcoded password in test.Even with the NOSONAR comment, it's better to avoid hardcoded passwords in source code, even for testing. Consider using an environment variable or a more secure approach for sensitive data in tests.
- password: 'test_password', //NOSONAR + password: 'mock-password', //NOSONARapp/routes/authenticated/dashboard/organization-settings/integrations.ts (1)
48-51: Limited error handling in API request.The error handling only checks for status code 400 to set
reconnect = true. Other error cases (404, 500, network errors) are silently ignored, which might lead to confusing UI state.Consider handling additional error cases or at least adding some logging:
} catch (err) { if ((err as AjaxError).status === 400) { reconnect = true; + } else if ((err as AjaxError).status === 404) { + // Handle not found case + } else { + // Log or handle other errors + console.error('Error fetching GitHub integration:', err); } }tests/integration/components/project-settings/integrations/service-now-test.js (1)
114-117: Use a regular expression literal instead of the RegExp constructor.The RegExp constructor requires escaping special characters in string literals, which can lead to hard-to-read code. A regular expression literal would be clearer.
- .hasAttribute( - 'href', - new RegExp('organization/settings/integrations', 'i') - ); + .hasAttribute( + 'href', + /organization\/settings\/integrations/i + );🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
tests/integration/components/organization/integrations/jira-account-test.js (1)
137-138: Use a regular expression literal instead of RegExp constructor.The code uses
new RegExp(...)when a regex literal would be clearer and avoid unnecessary escaping.- .hasAttribute( - 'href', - new RegExp('organization/settings/integrations', 'i') - ); + .hasAttribute( + 'href', + /organization\/settings\/integrations/i + );tests/integration/components/project-settings/integrations/jira-project-test.js (1)
389-389: Empty expect statement in test.The
assert.expect()call without a parameter doesn't serve a purpose. Either specify the expected number of assertions or remove this line.- assert.expect(); + // Remove this line or specify the expected number of assertionsapp/components/organization/integrations/service-now/index.ts (1)
185-186: Use optional chaining for error validation.Change to an optional chain to prevent potential runtime errors if
changeset.errorsis null or undefined.- if (changeset.errors && changeset.errors[0].validation) { + if (changeset.errors?.[0]?.validation) {🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/project-settings/integrations/github-project/index.hbs (1)
32-51: Consider adding data-test attribute to the 'no repos' text container.While the inner typography element has a data-test attribute, adding one to the parent AkStack would improve testability.
- <AkStack @alignItems='center' @spacing='0.5'> + <AkStack @alignItems='center' @spacing='0.5' data-test-projectSettings-integrations-githubProject-noReposContainer>app/components/project-settings/integrations/service-now/index.hbs (1)
120-125: Consider adding a more descriptive confirmation message.The current message might benefit from more specific wording about the consequences of removing the ServiceNow integration.
- @description={{t 'confirmBox.removeServiceNow'}} + @description={{t 'confirmBox.removeServiceNowWithConsequences'}}This assumes you'll add a new translation key that explains the impact of removal.
app/components/project-settings/integrations/jira-project/index.hbs (2)
42-56: Missing data-test attribute for no integration message.While other similar messages have data-test attributes, this typography element is missing one.
- <AkTypography @color='textSecondary' @variant='body2'> + <AkTypography @color='textSecondary' @variant='body2' data-test-prjSettings-integrations-jiraProject-noIntegrationText>
72-73: Different pattern for drawer save action compared to other integrations.Other integration components use a method reference, while this one uses
perform this.selectProject. Consider standardizing the approach across all integration components.- @drawerSaveAction={{perform this.selectProject}} + @drawerSaveAction={{this.selectProjectAction}}This assumes you'll create a method that performs the task, similar to the other integration components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/service-now.pngis excluded by!**/*.png
📒 Files selected for processing (84)
app/adapters/servicenow-config.ts(1 hunks)app/components/github-account/index.hbs(0 hunks)app/components/github-account/index.scss(0 hunks)app/components/jira-account/index.hbs(0 hunks)app/components/jira-account/index.scss(0 hunks)app/components/marketplace/integration-list/index.ts(1 hunks)app/components/organization/integrations/github-account/index.hbs(1 hunks)app/components/organization/integrations/github-account/index.ts(4 hunks)app/components/organization/integrations/index.hbs(1 hunks)app/components/organization/integrations/index.scss(1 hunks)app/components/organization/integrations/index.ts(1 hunks)app/components/organization/integrations/integrated-ui/index.hbs(1 hunks)app/components/organization/integrations/integrated-ui/index.scss(1 hunks)app/components/organization/integrations/integrated-ui/index.ts(1 hunks)app/components/organization/integrations/jira-account/index.hbs(1 hunks)app/components/organization/integrations/jira-account/index.ts(5 hunks)app/components/organization/integrations/service-now/index.hbs(1 hunks)app/components/organization/integrations/service-now/index.ts(1 hunks)app/components/organization/integrations/service-now/validator.ts(1 hunks)app/components/organization/settings-wrapper/index.ts(1 hunks)app/components/organization/settings/index.hbs(0 hunks)app/components/organization/settings/index.ts(1 hunks)app/components/project-settings/general-settings/github-project/index.hbs(0 hunks)app/components/project-settings/general-settings/index.hbs(1 hunks)app/components/project-settings/general-settings/jira-project/index.hbs(0 hunks)app/components/project-settings/general-settings/jira-project/index.scss(0 hunks)app/components/project-settings/header/index.ts(1 hunks)app/components/project-settings/integrations/config-drawer/index.hbs(1 hunks)app/components/project-settings/integrations/config-drawer/index.scss(1 hunks)app/components/project-settings/integrations/config-drawer/index.ts(1 hunks)app/components/project-settings/integrations/github-project/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/index.ts(9 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.ts(1 hunks)app/components/project-settings/integrations/index.hbs(1 hunks)app/components/project-settings/integrations/index.scss(1 hunks)app/components/project-settings/integrations/index.ts(1 hunks)app/components/project-settings/integrations/integrated-header/index.hbs(1 hunks)app/components/project-settings/integrations/integrated-header/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/index.ts(10 hunks)app/components/project-settings/integrations/jira-project/no-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/no-project/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.ts(1 hunks)app/components/project-settings/integrations/loading-ui/index.hbs(1 hunks)app/components/project-settings/integrations/loading-ui/index.ts(1 hunks)app/components/project-settings/integrations/risk-threshold/index.hbs(1 hunks)app/components/project-settings/integrations/risk-threshold/index.scss(1 hunks)app/components/project-settings/integrations/risk-threshold/index.ts(1 hunks)app/components/project-settings/integrations/service-now/index.hbs(1 hunks)app/components/project-settings/integrations/service-now/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/service-account.ts(1 hunks)app/controllers/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/enums.ts(1 hunks)app/models/servicenow-config.ts(1 hunks)app/router.ts(3 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/routes/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/styles/_component-variables.scss(2 hunks)app/templates/authenticated/dashboard/organization-settings/integrations.hbs(1 hunks)app/templates/authenticated/dashboard/project/settings/integrations.hbs(1 hunks)config/environment.js(2 hunks)mirage/factories/github-integration.ts(0 hunks)mirage/factories/jira-integration.ts(0 hunks)mirage/factories/jira-repo.ts(1 hunks)mirage/factories/servicenow-config.ts(1 hunks)mirage/scenarios/default.js(0 hunks)tests/integration/components/github-account-test.js(0 hunks)tests/integration/components/jira-account-test.js(0 hunks)tests/integration/components/organization/integrations/github-account-test.js(1 hunks)tests/integration/components/organization/integrations/jira-account-test.js(1 hunks)tests/integration/components/organization/integrations/service-now-test.js(1 hunks)tests/integration/components/project-settings/general-settings/github-project-test.js(0 hunks)tests/integration/components/project-settings/general-settings/jira-project-test.js(0 hunks)tests/integration/components/project-settings/integrations/github-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/jira-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/service-now-test.js(1 hunks)translations/en.json(8 hunks)translations/ja.json(8 hunks)types/irene/index.d.ts(1 hunks)
💤 Files with no reviewable changes (15)
- mirage/factories/github-integration.ts
- mirage/scenarios/default.js
- app/components/jira-account/index.hbs
- mirage/factories/jira-integration.ts
- app/components/project-settings/general-settings/jira-project/index.hbs
- app/components/project-settings/general-settings/jira-project/index.scss
- app/components/github-account/index.hbs
- app/components/jira-account/index.scss
- tests/integration/components/github-account-test.js
- app/components/github-account/index.scss
- app/components/organization/settings/index.hbs
- tests/integration/components/project-settings/general-settings/github-project-test.js
- tests/integration/components/project-settings/general-settings/jira-project-test.js
- app/components/project-settings/general-settings/github-project/index.hbs
- tests/integration/components/jira-account-test.js
🚧 Files skipped from review as they are similar to previous changes (48)
- app/routes/authenticated/dashboard/project/settings/integrations.ts
- app/controllers/authenticated/dashboard/organization-settings/service-account.ts
- app/components/project-settings/integrations/risk-threshold/index.scss
- app/components/organization/integrations/integrated-ui/index.hbs
- mirage/factories/jira-repo.ts
- types/irene/index.d.ts
- mirage/factories/servicenow-config.ts
- app/enums.ts
- app/templates/authenticated/dashboard/project/settings/integrations.hbs
- app/components/project-settings/integrations/risk-threshold/index.hbs
- app/components/project-settings/header/index.ts
- app/components/organization/settings-wrapper/index.ts
- app/components/organization/integrations/service-now/validator.ts
- app/components/project-settings/integrations/jira-project/no-project/index.hbs
- app/templates/authenticated/dashboard/organization-settings/integrations.hbs
- app/components/organization/integrations/index.scss
- app/components/project-settings/integrations/risk-threshold/index.ts
- app/components/project-settings/general-settings/index.hbs
- app/components/marketplace/integration-list/index.ts
- app/components/project-settings/integrations/jira-project/selected-project/index.ts
- app/components/organization/integrations/integrated-ui/index.scss
- app/components/organization/integrations/github-account/index.hbs
- app/components/project-settings/integrations/config-drawer/index.scss
- app/components/project-settings/integrations/jira-project/no-project/index.ts
- app/components/project-settings/integrations/loading-ui/index.ts
- app/components/organization/integrations/index.hbs
- app/components/project-settings/integrations/jira-project/selected-project/index.hbs
- app/components/project-settings/integrations/integrated-header/index.hbs
- app/components/project-settings/integrations/loading-ui/index.hbs
- app/components/project-settings/integrations/config-drawer/index.hbs
- app/components/project-settings/integrations/index.hbs
- config/environment.js
- app/controllers/authenticated/dashboard/organization-settings/integrations.ts
- app/components/project-settings/integrations/index.scss
- app/components/project-settings/integrations/config-drawer/index.ts
- app/components/organization/integrations/integrated-ui/index.ts
- app/models/servicenow-config.ts
- app/components/organization/integrations/jira-account/index.ts
- app/components/organization/settings/index.ts
- app/controllers/authenticated/dashboard/organization-settings/index.ts
- app/components/organization/integrations/index.ts
- app/components/project-settings/integrations/github-project/selected-repo/index.ts
- app/components/organization/integrations/jira-account/index.hbs
- app/router.ts
- app/components/project-settings/integrations/index.ts
- app/components/organization/integrations/github-account/index.ts
- app/components/project-settings/integrations/jira-project/index.ts
- app/components/project-settings/integrations/github-project/selected-repo/index.hbs
🧰 Additional context used
🪛 Biome (1.9.4)
tests/integration/components/project-settings/integrations/service-now-test.js
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
app/components/organization/integrations/service-now/index.ts
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/integration/components/project-settings/integrations/jira-project-test.js
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
tests/integration/components/project-settings/integrations/github-project-test.js
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (71)
app/components/project-settings/integrations/integrated-header/index.ts (5)
1-6: LGTM! Appropriate imports for the component.The imports cover all necessary dependencies for a Glimmer component with internationalization support.
7-15: Well-defined TypeScript interface with clear argument structure.The interface clearly defines the component's API with appropriate required and optional parameters.
17-21: Component structure follows best practices.The class properly extends Component with the defined signature type, and the tracked property will ensure reactivity for the dropdown positioning.
36-44: Action handlers are properly implemented.Both handlers correctly close the actions menu before triggering the passed-in actions.
57-61: Appropriate Glint type registration.The module declaration correctly registers the component for type checking in templates.
app/components/project-settings/integrations/github-project/index.ts (3)
78-92: Good addition of computed property for contextual header text.The
headerSubTextproperty effectively handles different states of the GitHub integration, providing appropriate text based on whether the user needs to reconnect, has no integration, or has an integrated account. This makes the UI more informative and user-friendly.
192-207: Improved error handling logic.The error handling has been enhanced with more specific checks for various error conditions. This will provide clearer feedback to users when integration issues occur.
305-311: Complete error handling for "Github not integrated".Good improvement in handling the "Github not integrated" error case, which now correctly sets the
noIntegrationflag and clears the GitHub repos array, ensuring consistent UI state.tests/integration/components/project-settings/integrations/github-project-test.js (1)
177-201: Well-structured test for non-integrated state.This test thoroughly verifies the GitHub component's rendering when no repositories are available, checking both the presence of integration instructions and the absence of integrated UI elements.
🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
tests/integration/components/organization/integrations/service-now-test.js (1)
192-213: Test for integrated state now properly calls assertion helper.The test now correctly calls the
assertIntegratedUIhelper function to verify the component's state when ServiceNow is integrated, addressing the issue from the previous review.app/routes/authenticated/dashboard/organization-settings/integrations.ts (1)
32-59: Well-structured model method with proper typing.The
modelmethod properly fetches integration data for the organization and handles potential errors. The return type is well-defined with theOrganizationSettingsIntegrationsRouteModeltype.app/controllers/authenticated/dashboard/project/settings/integrations.ts (1)
1-33: Well-structured controller with proper type definitions and breadcrumb setup.The controller follows Ember's conventions for defining routes and navigation. The breadcrumbs property is properly set up with appropriate internationalization and navigation structure for the integration settings page.
app/adapters/servicenow-config.ts (1)
26-28: LGTM! Method correctly sets the namespace for project-specific API calls.The setNestedUrlNamespace method correctly constructs the API path for project-specific ServiceNow configuration.
tests/integration/components/project-settings/integrations/service-now-test.js (1)
1-416: Comprehensive test suite with good coverage of component functionality.The test suite thoroughly checks all the main functionality of the ServiceNow integration component, including rendering states, configuration, validation, saving, and deletion operations. Good job using the test.each pattern for testing variation in error scenarios.
🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
tests/integration/components/organization/integrations/github-account-test.js (1)
1-260: Well-structured test suite with thorough coverage of GitHub integration features.The test cases appropriately cover all key user flows including:
- Initial rendering state
- Reconnection scenarios
- Integrated state display
- Disconnection with both success and error paths
- Integration URL redirection
The service stubs are appropriately implemented and the assertions thoroughly verify both UI elements and service interactions.
app/styles/_component-variables.scss (2)
648-651: New Integrated UI Variables AddedThis segment introduces the new CSS variables for the organization/integrations/integrated-ui component that replace the account-specific (GitHub/JIRA) styles. The variable names (
--organization-integrations-integrated-ui-border-radiusand--organization-integrations-integrated-ui-border-color) follow the established naming conventions and correctly utilize existing base variables.
1497-1511: New Config Drawer Variables for Integrations AddedThis block defines the styling for the project-settings/integrations/config-drawer component by setting variables for background color, box-shadow, border color, and footer border radius. The naming is clear, consistent, and aligns with how similar UI components are styled elsewhere in the file.
translations/en.json (4)
200-204: ServiceNow integration strings added correctly.The new confirmation messages for ServiceNow configuration follow the same pattern as the existing GitHub and JIRA integration messages, maintaining consistency in the UI.
609-613: ServiceNow integration UI strings follow existing patterns.The integration strings for ServiceNow follow the same pattern as GitHub and JIRA, with appropriate messaging for the integration status.
1045-1046: Service Now risk threshold string maintains consistency with existing templates.The risk threshold selection message follows the same pattern as other integration templates.
1455-1468: Comprehensive ServiceNow translation keys added.The ServiceNow section includes all the necessary translation keys for the integration UI, following a consistent pattern with other integration sections. Good job on maintaining consistency.
app/components/project-settings/integrations/service-now/index.ts (9)
14-18: Interface properly defined with optional project argument.The component signature interface is well-structured, following TypeScript best practices with optional properties correctly typed.
20-31: Component declaration with proper service injections and tracked properties.The component follows Ember best practices with proper service injections and tracked properties for managing component state.
41-69: Well-structured computed properties.The component has well-organized computed properties with clear naming that makes the code readable and maintainable.
105-129: Comprehensive error handling in fetchServiceNowConfig task.The task properly handles different error scenarios, including 404 responses and integration checks. Good job on handling specific error messages and setting appropriate state flags.
131-147: Good implementation of deleteServiceNowConfig task.The task correctly handles the deletion of ServiceNow configuration, manages UI state, and provides appropriate user feedback with error handling.
149-203: Complex selectRiskThreshold task with thorough error handling.The task appropriately handles both creation and update scenarios for ServiceNow configuration. The error handling is comprehensive, with specific handling for validation errors and integration issues.
182-193: Good error handling for validation errors.The component correctly handles validation errors for risk threshold by checking the source pointer and displaying appropriate error messages.
206-210: Proper Glint registry declaration.The module declaration for Glint ensures type safety when using the component in templates, following best practices for Ember TypeScript development.
177-179:❓ Verification inconclusive
Potential issue with deleted record handling.
The condition checking
dirtyType === 'deleted'and then unloading the record may cause issues if the model's state tracking implementation changes in the future.
🏁 Script executed:
#!/bin/bash # Check if similar pattern is used elsewhere in the codebase for consistency echo "Looking for other instances of dirtyType === 'deleted' pattern:" rg "dirtyType\s*===\s*('|\")deleted" --type tsLength of output: 193
Action: Please verify the logic for handling a "deleted" state in the ServiceNow integration.
The code snippet that unloads the record when it detects a
dirtyTypeof'deleted'appears to be the only instance of this pattern in the codebase. Given this isolated usage, there's a potential risk if the model's state tracking implementation changes in the future. Please ensure that:
- This logic in
app/components/project-settings/integrations/service-now/index.ts(lines 177–179) remains valid as state management evolves.- If the model's state tracking is updated, similar checks are either adjusted accordingly or centralized to maintain consistency across the codebase.
tests/integration/components/organization/integrations/jira-account-test.js (3)
11-28: Well-structured service stubs for testing.The service stubs for OrganizationMe and Notifications are well-designed for testing purposes, capturing the necessary behavior without complex implementation.
30-50: Useful test helper for header action items.The
assertIntegratedHeaderActionItemshelper function improves test readability and reduces code duplication, making the tests more maintainable.
161-403: Comprehensive test coverage for JIRA account integration.The tests thoroughly cover the component's functionality, including rendering states, user interactions, error conditions, and success scenarios. This ensures the component behaves as expected under various conditions.
tests/integration/components/project-settings/integrations/jira-project-test.js (6)
16-27: Minimal notification stub for testing.The NotificationsStub effectively captures the notifications for testing verification without unnecessary complexity.
30-50: Useful helper function for repeated assertions.The
assertIntegratedHeaderActionItemshelper improves test readability and reduces code duplication, similar to the previous test file.
52-54: Utility function for risk labels.The
getAnalysisRisklabelfunction encapsulates the risk label logic, making the tests more readable and maintainable.
241-269: Thorough test for form validation errors.The test properly verifies error handling for different validation scenarios, ensuring the component provides appropriate feedback when required fields are missing.
230-404: Comprehensive test for JIRA project integration.The test thoroughly covers the integration functionality, including form validation, project selection, threshold selection, and project updates.
462-553: Complete test for editing existing JIRA project.The test verifies the full workflow of editing an existing JIRA project integration, ensuring that all UI elements update correctly and the proper API calls are made.
app/routes/authenticated/dashboard/organization-settings/index.ts (2)
11-15: Well-structured type definition.Adding the
OrganizationSettingsRouteModeltype improves code clarity and type safety. This provides better organization to the route data structure.
21-30: Clean implementation of the model method.The refactored model method is more focused and straightforward, returning just the essential data without unnecessary GitHub integration logic. This simplification makes the code more maintainable.
app/components/organization/integrations/service-now/index.hbs (5)
1-7: Appropriate title component with translation support.Good use of the
AkTypographycomponent with proper attributes and data test ID for the ServiceNow integration title.
9-18: Well-implemented integrated UI state.The conditional rendering for the integrated state is clean and passes all necessary properties to the
IntegratedUicomponent.
20-27: Clear description with proper styling.Using the
AkTypographycomponent with appropriate variant and color for the integration description.
29-94: Well-structured form implementation with proper validation.The form is well-organized with:
- Appropriate field labels and placeholders using translations
- Error handling for each input field
- Good use of AkStack for layout organization
- Clear dropdown implementation for ServiceNow table selection
- Properly configured integration button with loading state
This provides a user-friendly interface for setting up the ServiceNow integration.
97-103: Properly implemented confirmation dialog.The ConfirmBox component is well configured with appropriate delegate, conditional display, and translation keys.
app/components/organization/integrations/service-now/index.ts (7)
11-14: Proper import and use of validator.The component correctly imports and uses the ServiceNow validator from the dedicated file for form validation.
Also applies to: 19-19
21-50: Well-defined type definitions for component data.The TypeScript interfaces and types for ServiceNow integration data are clearly defined and properly structured.
52-57: Component class with properly injected services.All necessary services are properly injected and declared with correct types.
93-104: Well-implemented table items getter.The
snTableItemsgetter provides a clean interface for the dropdown options using constants from the ENUMS object.However, there seems to be a potential issue with both options having the same value:
- label: 'sn_vul_vulnerable_item', - value: ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_APP_VULNERABLE_ITEM, + label: 'sn_vul_vulnerable_item', + value: ENUMS.SERVICE_NOW_TABLE_SELECTION.SN_VUL_VULNERABLE_ITEM,Double-check if this is intended or if they should have different enum values.
141-163: Comprehensive error handling in ServiceNow integration check.The task properly handles the API response and potential 404 errors, setting the component state accordingly.
181-225: Well-implemented integration task with proper validation and error handling.The
integrateServiceNowtask includes:
- Form validation
- Error feedback for invalid data
- Proper data formatting before submission
- Comprehensive error handling for API responses
- Success notification and analytics tracking
This provides a robust implementation for the integration process.
🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
238-242: Proper Glint module declaration.The Glint module declaration ensures proper type checking for the component in templates.
translations/ja.json (3)
201-204: Added ServiceNow confirmation messages.The confirmation messages for removing and revoking ServiceNow integrations have been properly added to the translations.
1455-1467: Complete ServiceNow translation keys.A comprehensive set of translation keys has been added for the ServiceNow integration, covering all UI elements and messages. These include instance URL fields, table selection, integration status, and risk threshold messages.
Note that these translations appear to be in English despite being in the Japanese translation file. Ensure that proper Japanese translations are planned for these strings in the future.
550-551: Consistent updates to GitHub and JIRA messages.The integration messages for GitHub and JIRA have been updated with "Change by" to maintain consistency across all integration types.
Also applies to: 658-659
app/components/project-settings/integrations/github-project/index.hbs (7)
1-5: Great use of AkStack for consistent layout.The choice of AkStack with full width and column direction provides a solid foundation for the integration UI. The data-test attribute is properly implemented for automated testing.
6-12: Nice component abstraction for header elements.The
IntegratedHeadercomponent effectively encapsulates common header functionality across different integration types, promoting code reuse and consistent UI patterns.
14-16: Well-handled loading state.The loading UI component provides good user feedback during asynchronous operations.
56-61: Using isRunning for loadingDrawerSaveAction is a good practice.Leveraging the task state for controlling UI loading states ensures consistency between the UI and the underlying operations.
72-91: Repository selection is well-structured.The repository selection uses appropriate stack layouts and typography to provide a clear hierarchy. The AkSelect component is properly configured with options, selection handling, and placeholder text.
93-110: Good use of consistent pattern for threshold selection.This follows the same pattern as repository selection, maintaining consistency in the UI.
117-122: Properly implemented confirmation box for deletion.The ConfirmBox component is appropriately configured with active state, description, and disabled state during deletion process.
app/components/project-settings/integrations/service-now/index.hbs (4)
1-12: Consistent structure with other integration components.The ServiceNow integration follows the same pattern as GitHub and JIRA integrations, using AkStack and IntegratedHeader consistently.
18-31: Good separation of concerns for risk threshold display.The risk threshold component is nicely separated, allowing for consistent display across different integration types.
33-74: Well-structured conditional rendering for different states.The component properly handles different states: no access, no integration, and default state. Each state provides appropriate UI feedback and actions.
78-117: Config drawer implementation is consistent with other integrations.Using the same pattern for configuration drawers across integrations helps maintain a consistent user experience.
app/components/project-settings/integrations/jira-project/index.hbs (4)
1-12: Consistent implementation with other integration components.The JIRA integration component follows the same structure as the GitHub and ServiceNow components, ensuring a unified experience across integrations.
33-41: Good error handling for access restrictions.The warning icon and error message provide clear feedback when the user doesn't have sufficient permissions.
127-136: Great addition of explanatory note for JIRA selection.The additional note with formatted text helps clarify the JIRA account selection process, improving user understanding.
141-146: Consistent implementation of confirmation box.The ConfirmBox implementation matches the pattern used in other integration components.
tests/integration/components/organization/integrations/jira-account-test.js
Outdated
Show resolved
Hide resolved
Yibaebi
force-pushed
the
PD-1715-service-now-integration-ui-changes
branch
from
6ee6554 to
d738607
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/adapters/servicenow-config.ts (1)
13-24:⚠️ Potential issueAdd error handling to the updateRecord method.
The updateRecord method doesn't include any error handling, which could lead to unhandled promise rejections. This issue was previously identified in a past review but hasn't been addressed.
updateRecord<K extends keyof ModelRegistry>( _: Store, _type: ModelSchema<K>, snapshot: Snapshot<K> ) { const { riskThreshold } = snapshot.record as ServiceNowConfigModel; const url = this._buildURL(); - return this.ajax(url, 'POST', { - data: { risk_threshold: riskThreshold }, - }); + return this.ajax(url, 'POST', { + data: { risk_threshold: riskThreshold }, + }).catch(error => { + console.error('Error updating ServiceNow config:', error); + throw error; + }); }
🧹 Nitpick comments (29)
tests/integration/components/organization/integrations/github-account-test.js (2)
229-230: Consider using a more explicit approach for URL mockingThe current implementation uses a hash URL with a comment to prevent browser refresh, but this is somewhat brittle. Consider using a more robust approach such as mocking window.location or using a service wrapper for navigation that can be stubbed during tests.
255-256: Ensure cleanup is comprehensiveThe test cleans up by resetting the hash, which is good practice. Consider moving this to an afterEach hook to ensure cleanup happens even if the test fails.
hooks.beforeEach(async function () { this.server.createList('organization', 1); await this.owner.lookup('service:organization').load(); this.owner.register('service:me', OrganizationMeStub); this.owner.register('service:notifications', NotificationsStub); }); + +hooks.afterEach(function() { + // Clean up any navigation changes + window.location.hash = ''; +});tests/integration/components/project-settings/integrations/github-project-test.js (9)
29-43: Fix typo in function name.The function name
creatOrgRepois missing an 'e' and should becreateOrgRepoto improve code readability and follow naming conventions.-const creatOrgRepo = (propsOverride = {}) => { +const createOrgRepo = (propsOverride = {}) => {
117-117: Update function name reference.Update the reference to the function after renaming from
creatOrgRepotocreateOrgRepo.- const orgGithubRepos = [1, 2, 3].map(() => creatOrgRepo()); + const orgGithubRepos = [1, 2, 3].map(() => createOrgRepo());
489-489: Update function name reference.Update the reference to the function after renaming from
creatOrgRepotocreateOrgRepo.- creatOrgRepo({ + createOrgRepo({
68-69: Improve readability of the getAnalysisRisklabel function.The function uses nested function calls which makes it harder to understand at a glance. Consider restructuring for better readability.
-const getAnalysisRisklabel = (risk) => - analysisRiskStatus([risk, ENUMS.ANALYSIS.COMPLETED, false]).label; +const getAnalysisRisklabel = (risk) => { + const analysisStatus = analysisRiskStatus([risk, ENUMS.ANALYSIS.COMPLETED, false]); + return analysisStatus.label; +};
16-27: Add reset method to NotificationsStub service.To ensure test isolation, consider adding a
reset()method to clear any stored messages between tests. This would prevent potential test leakage where messages from one test could affect assertions in another.class NotificationsStub extends Service { errorMsg = null; successMsg = null; error(msg) { this.errorMsg = msg; } success(msg) { this.successMsg = msg; } + + reset() { + this.errorMsg = null; + this.successMsg = null; + } }
78-81: Reset notification messages in beforeEach hook.To ensure test isolation, reset the notification messages at the start of each test. This prevents message leakage between tests.
hooks.beforeEach(async function () { this.owner.register('service:notifications', NotificationsStub); this.notifyService = this.owner.lookup('service:notifications'); + this.notifyService.reset();
30-31: Use seeded faker data for deterministic tests.To improve test determinism and prevent flaky tests due to random data, consider using a seeded faker instance.
+// Set a fixed seed for predictable test data +faker.seed(123); + const creatOrgRepo = (propsOverride = {}) => { const repo = faker.lorem.slug(); const account = faker.lorem.slug();
269-311: Add assertions for error messages.The test case for error handling doesn't verify the presence of error messages in the UI, only in the notification service. Consider adding assertions to check if error messages are displayed to the user.
For each error condition, add an assertion like:
assert.dom('[data-test-error-message]').exists().containsText(message());This would ensure that error messages are properly displayed to users in the UI, not just logged to the notifications service.
1-570: Add accessibility tests for UI components.The test suite thoroughly covers functionality but doesn't include any accessibility (a11y) tests. For UI components, it's important to test that they're accessible to all users.
Consider adding a test case using ember-a11y-testing:
test('it meets accessibility standards', async function(assert) { await render(hbs`<ProjectSettings::Integrations::GithubProject @project={{this.project}} />`); // Test that the component is accessible await a11yAudit(); assert.ok(true, 'no a11y errors found'); });This would ensure that the component meets basic accessibility standards.
🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
tests/integration/components/project-settings/integrations/service-now-test.js (2)
116-116: Use a regular expression literal instead of RegExp constructor.Using a regular expression literal is more efficient and cleaner than using the RegExp constructor.
- 'href', - new RegExp('organization/settings/integrations', 'i') + 'href', + /organization\/settings\/integrations/i🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
167-204: Simplify test.each usage with a single test case.The
test.eachis being used with only a single test case, which adds unnecessary complexity. Consider using a regular test function instead.- test.each( - 'It shows an error message when no threshold is selected after clicking save in config drawer', - [ - [ - { - risk_threshold: ['This field may not be null.'], - }, - () => t('invalidRisk'), - ], - ], - async function (assert, [error, message]) { + test('It shows an error message when no threshold is selected after clicking save in config drawer', async function (assert) { + const error = { + risk_threshold: ['This field may not be null.'], + }; + const message = () => t('invalidRisk');app/components/project-settings/integrations/index.ts (2)
37-49: Improve error handling in fetchProfile task.The current error handling silently sets the profile to null without logging the error or notifying the user. This could make debugging difficult if there are issues with profile fetching.
fetchProfile = task(async () => { try { const profileId = this.args.project?.activeProfileId; this.profile = await waitForPromise( this.store.findRecord('profile', String(profileId)) ); } catch (e) { this.profile = null; + console.error('Error fetching profile:', e); return; } });
33-35: Add null check for project in getter.The project getter doesn't handle the case where
this.args.projectis null or undefined, which could lead to errors when accessing properties.get project() { - return this.args.project; + return this.args.project || null; }tests/integration/components/project-settings/integrations/jira-project-test.js (2)
137-137: Use a regular expression literal instead of the RegExp constructor.Replace the constructor call with a literal for improved readability and easier static analysis:
- new RegExp('organization/settings/integrations', 'i') + /organization\/settings\/integrations/i🧰 Tools
🪛 Biome (1.9.4)
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
389-389: Specify an argument for assert.expect().Calling
assert.expect()with no argument won’t help QUnit verify the number of assertions. Provide a numeric argument to ensure correctness:- assert.expect(); + assert.expect(6); // Adjust to the actual number of assertionsapp/components/project-settings/integrations/integrated-header/index.ts (1)
41-44: Consider adding a confirmation prompt before deleting.Currently, the
handleDeleteActiontriggersthis.args.deleteAction()without user confirmation. For destructive actions, it's often best practice to display a confirmation modal or message to prevent accidental deletions.app/components/organization/integrations/service-now/index.hbs (2)
9-19: Ensure adequate loading and error feedback when integrated.Displaying the integrated UI is good, but if the network is slow or there's an error, consider showing an appropriate loading indicator or error message. This will help users understand the state of the integration process.
29-94: Validate user input earlier in the form.Each field is properly bound to the changeset, which defers validation until submission. Consider performing live validation (e.g., on blur) to provide more immediate feedback and improve user experience.
app/components/organization/integrations/service-now/index.ts (2)
141-163: Add logging or telemetry for unexpected errors.Within the
checkServiceNowIntegrationtask, you handle a404status but skip logging other potential errors. Adding a catch-all logging mechanism (e.g., Sentry or console error) could help diagnose unexpected failures.
185-185: Use optional chaining for safer condition checks.Here, manually checking
changeset.errors && changeset.errors[0].validationcan be replaced with optional chaining for improved readability and safety.- if (changeset.errors && changeset.errors[0].validation) { + if (changeset.errors?.[0]?.validation) { this.notify.error(changeset.errors[0].validation, ENV.notifications); }🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/components/project-settings/integrations/github-project/index.ts (2)
193-207: Error handling logic could be more streamlinedThe error handling has several conditional blocks checking for specific error messages. Consider refactoring this logic to use a more systematic approach, such as a switch statement or mapping error messages to handlers.
- const errorDetail = err?.errors?.[0]?.detail; - - // If Repos exist and error is not integrated, then something went wrong with the integration - if (errorDetail === 'Github not integrated' && this.hasGitHubProject) { - this.currentGithubRepo = null; - this.reconnect = true; - - return; - } - - if (errorDetail === 'Github not integrated') { - this.currentGithubRepo = null; - - return; - } - - if (errorDetail === 'Github integration failed') { - this.reconnect = true; - - return; - } + const errorDetail = err?.errors?.[0]?.detail; + this.currentGithubRepo = null; + + // Handle different error scenarios + switch(errorDetail) { + case 'Github not integrated': + if (this.hasGitHubProject) { + this.reconnect = true; + } + return; + + case 'Github integration failed': + this.reconnect = true; + return; + }
234-248: Improve error handling consistency in fetchGithubReposSimilarly to the previous comment, the error handling here could be refactored for better readability and maintainability.
- const errorStatus = error?.status; - const errorDetail = error.payload.detail; - - if (errorStatus === 400 && errorDetail === 'Github integration failed') { - this.reconnect = true; - - return; - } - - if (errorStatus === 404 && errorDetail === 'Github not integrated') { - this.noIntegration = true; - this.githubRepos = []; - - return; - } + const errorStatus = error?.status; + const errorDetail = error.payload.detail; + + // Handle specific error cases with a more structured approach + if (errorStatus === 400 && errorDetail === 'Github integration failed') { + this.reconnect = true; + return; + } else if (errorStatus === 404 && errorDetail === 'Github not integrated') { + this.noIntegration = true; + this.githubRepos = []; + return; + }tests/integration/components/organization/integrations/service-now-test.js (5)
176-176: Missing expected assertion count.The
assert.expect()method is called without arguments, but it should specify the number of assertions that will be executed in the test. This helps ensure all expected assertions are actually run.- assert.expect(); + assert.expect(9); // Update with the actual number of assertions in this test
192-192: Missing expected assertion count.The
assert.expect()method is called without arguments, but it should specify the number of assertions that will be executed in the test.- assert.expect(); + assert.expect(7); // Update with the actual number of assertions in this test
247-249: Minor typo in variable name.There's a typo in the variable name "disconnecBtn" (missing "t").
- const disconnecBtn = find( + const disconnectBtn = find( '[data-test-orgIntegrations-serviceNow-integratedHeader] [data-test-orgIntegrations-integratedUI-disconnectBtn]' );
251-251: Update variable reference.Update the variable reference to match the fixed variable name.
- assert.dom(disconnecBtn).isNotDisabled().hasText(t('disconnect')); + assert.dom(disconnectBtn).isNotDisabled().hasText(t('disconnect'));
253-253: Update variable reference.Update the variable reference to match the fixed variable name.
- await click(disconnecBtn); + await click(disconnectBtn);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/service-now.pngis excluded by!**/*.png
📒 Files selected for processing (84)
app/adapters/servicenow-config.ts(1 hunks)app/components/github-account/index.hbs(0 hunks)app/components/github-account/index.scss(0 hunks)app/components/jira-account/index.hbs(0 hunks)app/components/jira-account/index.scss(0 hunks)app/components/marketplace/integration-list/index.ts(1 hunks)app/components/organization/integrations/github-account/index.hbs(1 hunks)app/components/organization/integrations/github-account/index.ts(4 hunks)app/components/organization/integrations/index.hbs(1 hunks)app/components/organization/integrations/index.scss(1 hunks)app/components/organization/integrations/index.ts(1 hunks)app/components/organization/integrations/integrated-ui/index.hbs(1 hunks)app/components/organization/integrations/integrated-ui/index.scss(1 hunks)app/components/organization/integrations/integrated-ui/index.ts(1 hunks)app/components/organization/integrations/jira-account/index.hbs(1 hunks)app/components/organization/integrations/jira-account/index.ts(5 hunks)app/components/organization/integrations/service-now/index.hbs(1 hunks)app/components/organization/integrations/service-now/index.ts(1 hunks)app/components/organization/integrations/service-now/validator.ts(1 hunks)app/components/organization/settings-wrapper/index.ts(1 hunks)app/components/organization/settings/index.hbs(0 hunks)app/components/organization/settings/index.ts(1 hunks)app/components/project-settings/general-settings/github-project/index.hbs(0 hunks)app/components/project-settings/general-settings/index.hbs(1 hunks)app/components/project-settings/general-settings/jira-project/index.hbs(0 hunks)app/components/project-settings/general-settings/jira-project/index.scss(0 hunks)app/components/project-settings/header/index.ts(1 hunks)app/components/project-settings/integrations/config-drawer/index.hbs(1 hunks)app/components/project-settings/integrations/config-drawer/index.scss(1 hunks)app/components/project-settings/integrations/config-drawer/index.ts(1 hunks)app/components/project-settings/integrations/github-project/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/index.ts(9 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.ts(1 hunks)app/components/project-settings/integrations/index.hbs(1 hunks)app/components/project-settings/integrations/index.scss(1 hunks)app/components/project-settings/integrations/index.ts(1 hunks)app/components/project-settings/integrations/integrated-header/index.hbs(1 hunks)app/components/project-settings/integrations/integrated-header/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/index.ts(10 hunks)app/components/project-settings/integrations/jira-project/no-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/no-project/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.ts(1 hunks)app/components/project-settings/integrations/loading-ui/index.hbs(1 hunks)app/components/project-settings/integrations/loading-ui/index.ts(1 hunks)app/components/project-settings/integrations/risk-threshold/index.hbs(1 hunks)app/components/project-settings/integrations/risk-threshold/index.scss(1 hunks)app/components/project-settings/integrations/risk-threshold/index.ts(1 hunks)app/components/project-settings/integrations/service-now/index.hbs(1 hunks)app/components/project-settings/integrations/service-now/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/service-account.ts(1 hunks)app/controllers/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/enums.ts(1 hunks)app/models/servicenow-config.ts(1 hunks)app/router.ts(3 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/routes/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/styles/_component-variables.scss(2 hunks)app/templates/authenticated/dashboard/organization-settings/integrations.hbs(1 hunks)app/templates/authenticated/dashboard/project/settings/integrations.hbs(1 hunks)config/environment.js(2 hunks)mirage/factories/github-integration.ts(0 hunks)mirage/factories/jira-integration.ts(0 hunks)mirage/factories/jira-repo.ts(1 hunks)mirage/factories/servicenow-config.ts(1 hunks)mirage/scenarios/default.js(0 hunks)tests/integration/components/github-account-test.js(0 hunks)tests/integration/components/jira-account-test.js(0 hunks)tests/integration/components/organization/integrations/github-account-test.js(1 hunks)tests/integration/components/organization/integrations/jira-account-test.js(1 hunks)tests/integration/components/organization/integrations/service-now-test.js(1 hunks)tests/integration/components/project-settings/general-settings/github-project-test.js(0 hunks)tests/integration/components/project-settings/general-settings/jira-project-test.js(0 hunks)tests/integration/components/project-settings/integrations/github-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/jira-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/service-now-test.js(1 hunks)translations/en.json(8 hunks)translations/ja.json(8 hunks)types/irene/index.d.ts(1 hunks)
💤 Files with no reviewable changes (15)
- mirage/factories/github-integration.ts
- mirage/factories/jira-integration.ts
- app/components/project-settings/general-settings/jira-project/index.scss
- app/components/project-settings/general-settings/jira-project/index.hbs
- mirage/scenarios/default.js
- app/components/jira-account/index.scss
- app/components/organization/settings/index.hbs
- app/components/jira-account/index.hbs
- tests/integration/components/github-account-test.js
- app/components/github-account/index.hbs
- app/components/project-settings/general-settings/github-project/index.hbs
- tests/integration/components/project-settings/general-settings/jira-project-test.js
- app/components/github-account/index.scss
- tests/integration/components/jira-account-test.js
- tests/integration/components/project-settings/general-settings/github-project-test.js
🚧 Files skipped from review as they are similar to previous changes (51)
- app/components/project-settings/integrations/risk-threshold/index.scss
- app/components/organization/integrations/index.scss
- mirage/factories/servicenow-config.ts
- app/components/project-settings/integrations/risk-threshold/index.hbs
- app/components/organization/integrations/service-now/validator.ts
- app/templates/authenticated/dashboard/organization-settings/integrations.hbs
- app/enums.ts
- types/irene/index.d.ts
- app/routes/authenticated/dashboard/organization-settings/integrations.ts
- app/templates/authenticated/dashboard/project/settings/integrations.hbs
- app/components/project-settings/header/index.ts
- app/controllers/authenticated/dashboard/organization-settings/index.ts
- app/controllers/authenticated/dashboard/organization-settings/service-account.ts
- app/components/project-settings/integrations/jira-project/no-project/index.hbs
- app/components/project-settings/integrations/index.scss
- app/components/project-settings/integrations/jira-project/no-project/index.ts
- mirage/factories/jira-repo.ts
- app/components/marketplace/integration-list/index.ts
- app/components/project-settings/integrations/loading-ui/index.ts
- app/components/project-settings/integrations/loading-ui/index.hbs
- app/controllers/authenticated/dashboard/project/settings/integrations.ts
- app/components/project-settings/integrations/integrated-header/index.hbs
- app/components/organization/integrations/integrated-ui/index.hbs
- app/components/project-settings/integrations/config-drawer/index.hbs
- app/components/organization/integrations/integrated-ui/index.ts
- app/components/project-settings/integrations/risk-threshold/index.ts
- app/components/organization/settings/index.ts
- app/router.ts
- app/components/project-settings/integrations/jira-project/selected-project/index.ts
- app/components/organization/integrations/index.hbs
- app/components/project-settings/integrations/config-drawer/index.ts
- app/components/project-settings/general-settings/index.hbs
- app/components/project-settings/integrations/github-project/selected-repo/index.hbs
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/organization/integrations/jira-account/index.hbs
- app/components/project-settings/integrations/jira-project/selected-project/index.hbs
- config/environment.js
- app/components/organization/integrations/index.ts
- app/models/servicenow-config.ts
- app/components/project-settings/integrations/config-drawer/index.scss
- app/components/project-settings/integrations/index.hbs
- app/components/organization/settings-wrapper/index.ts
- app/components/organization/integrations/github-account/index.ts
- app/components/project-settings/integrations/jira-project/index.ts
- app/components/organization/integrations/github-account/index.hbs
- app/components/project-settings/integrations/github-project/selected-repo/index.ts
- app/routes/authenticated/dashboard/project/settings/integrations.ts
- app/components/organization/integrations/jira-account/index.ts
- translations/ja.json
- app/controllers/authenticated/dashboard/organization-settings/integrations.ts
- app/components/organization/integrations/integrated-ui/index.scss
🧰 Additional context used
🪛 Biome (1.9.4)
tests/integration/components/project-settings/integrations/github-project-test.js
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
tests/integration/components/project-settings/integrations/jira-project-test.js
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
app/components/organization/integrations/service-now/index.ts
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/integration/components/project-settings/integrations/service-now-test.js
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (32)
tests/integration/components/organization/integrations/github-account-test.js (3)
1-260: Well-structured integration tests with comprehensive coverageThis file provides excellent test coverage for the GitHub account integration component, properly testing various scenarios including non-integrated state, reconnection attempts, integrated state, and disconnect workflows.
144-222: Good use of test.each for testing both success and failure scenariosThe parametrized testing approach for the disconnect functionality is well-implemented, testing both the successful disconnection flow and error handling.
36-42: Consistent organization service setupThe test setup follows best practices by creating test data and registering stub services. This ensures tests are isolated and deterministic.
app/adapters/servicenow-config.ts (2)
26-28: LGTM: Proper method implementation for dynamic namespace setting.This method properly sets the namespace for the API path based on the provided project ID.
31-35: LGTM: Correct registry interface declaration.The adapter registry declaration correctly maps the 'servicenow-config' key to the ServiceNowConfigAdapter type, ensuring proper type safety.
tests/integration/components/project-settings/integrations/service-now-test.js (2)
206-278: LGTM: Comprehensive test for saving a selected threshold.This test thoroughly verifies the behavior when selecting and saving a risk threshold, including checking UI changes, asserting proper API requests, and validating success notifications.
280-341: LGTM: Well-structured test for deleting a risk threshold.This test properly verifies the deletion flow, including UI interactions, confirmation dialog, and post-deletion state verification.
app/styles/_component-variables.scss (2)
648-650: LGTM: Good abstraction for integration UI variables.The new variables for organization integrations UI follow the established pattern and provide a consistent way to style integration components.
1497-1510: LGTM: Well-structured variables for integration config drawer.The new variables for project settings integration config drawer follow the component naming conventions and provide all necessary styling properties.
translations/en.json (1)
1-1826: All newly added translations look consistent.app/components/project-settings/integrations/service-now/index.ts (1)
23-23: Missing type import for NotificationService.The NotificationService type is referenced but not imported, which may cause TypeScript compilation issues. Please add:
+ import type NotificationService from 'irene/services/notifications';tests/integration/components/organization/integrations/jira-account-test.js (1)
382-402: Refactor the commented-out assertions to handle asynchronous logic correctly.Use Ember’s async test helpers (e.g.
await settled(),waitUntil) to ensure the test waits for all concurrency tasks to complete, rather than commenting out the assertions.app/components/project-settings/integrations/integrated-header/index.ts (1)
22-34: Looks clean and consistent.The
headerActionsListgetter succinctly returns localized labels and properly references the action handlers. Overall, this approach is straightforward and maintainable.app/components/organization/integrations/service-now/index.ts (1)
199-210: Good integration of analytics tracking.The analytics call via
triggerAnalyticson successful integration is helpful for measuring usage. No concerns here.app/components/project-settings/integrations/github-project/index.hbs (3)
1-52: Well-structured GitHub integration UI with clear state handlingThe template uses a clean structure with conditional rendering to handle different states (loading, repository selected, CTA needed, or no integration). The use of dedicated components like
IntegratedHeader,LoadingUi, andSelectedRepopromotes reusability across integration UI components.
54-114: Config drawer implementation follows good component design practicesThe configuration drawer has well-organized sections for repository and threshold selection, with appropriate conditional rendering based on the availability of GitHub repositories. The component has clear data test attributes for testing, and uses proper translations for all text elements.
116-122: Confirm box implementation looks goodThe confirm box is properly implemented with the necessary properties for handling deletion confirmation, including the disabled state during deletion operation and clear description text.
app/components/project-settings/integrations/github-project/index.ts (4)
18-24: Component interface naming updated to reflect new locationGood update to the interface name to align with the component's new location in the project structure, moving from general settings to integrations.
35-39: Enhanced state management with descriptive computed propertiesThe addition of
noIntegrationproperty and new computed properties likeshowHeaderActions,headerSubText,noIntegrationOrNoRepoSubtext, andshowSelectPrjCTAgreatly improves the component's ability to handle different states and provide appropriate UI feedback.Also applies to: 74-139
282-311: Enhanced error handling in selectProject taskThe error handling has been improved with more specific checks for different error conditions. The code now correctly sets
noIntegrationand clearsgithubReposwhen GitHub is not integrated.
337-341: Updated component registry with the new namingGood update to the Glint registry with the new component name to ensure proper type checking.
app/components/project-settings/integrations/service-now/index.hbs (3)
1-75: Well-structured ServiceNow integration UI with comprehensive state handlingThe template provides clear UI for different states of the ServiceNow integration:
- Shows loading state during configuration fetch
- Displays risk threshold when configuration is available
- Shows access warnings when permissions are insufficient
- Provides guidance when no integration exists
- Offers a button to select threshold when needed
The UI structure is consistent with other integration components, providing a unified user experience.
77-117: Config drawer implementation follows established patternThe configuration drawer for risk threshold follows the same pattern as other integration components, with proper titling, state management, and user selection components.
119-125: Confirm box implementation is consistentThe confirmation box for deleting ServiceNow configuration is implemented consistently with other integration components.
app/components/project-settings/integrations/jira-project/index.hbs (3)
1-64: Well-organized JIRA integration UI with comprehensive state handlingThe template effectively handles various states:
- Shows loading state when fetching JIRA information
- Displays the selected project when available
- Provides a selection button when no project is selected
- Shows access warnings when permissions are insufficient
- Offers guidance when no integration exists
The conditional structure is clear and follows the same pattern as other integration templates.
66-138: Config drawer implementation with extra note sectionThe configuration drawer includes both repository and threshold selection, along with an additional note section separated by a divider. This provides important contextual information to users about the JIRA account selection process.
140-146: Confirm box implementation is consistentThe confirmation box for deleting JIRA integration is implemented consistently with other integration components.
tests/integration/components/organization/integrations/service-now-test.js (5)
191-212: The integrated state test looks good.The test correctly uses the
assertIntegratedUIhelper to verify the component's integrated state, addressing the issue from the previous review.
197-197: Security comment in test code is appropriate.Good use of the
//NOSONARcomment to indicate that the hardcoded password is acceptable in test code.
214-295: Well-structured test for disconnect functionality.The test comprehensively checks both success and failure scenarios when disconnecting ServiceNow integration, including validation of UI state and notification messages.
297-318: Good validation test implementation.The test correctly verifies that form validation works, checking that the input field gets the error class when validation fails.
320-455: Comprehensive integration test.The test covers multiple scenarios including successful integration and different error cases. It correctly validates both the UI state and notification messages.
tests/integration/components/project-settings/integrations/github-project-test.js
Show resolved
Hide resolved
Yibaebi
force-pushed
the
PD-1715-service-now-integration-ui-changes
branch
from
d738607 to
d8644f4
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/components/project-settings/integrations/service-now/index.ts (1)
23-23:⚠️ Potential issueMissing type import for NotificationService.
The NotificationService type is used but not imported, which may cause TypeScript compilation issues.
Add the following import at the top of the file:
+ import type NotificationService from 'irene/services/notifications';
🧹 Nitpick comments (13)
tests/integration/components/project-settings/integrations/github-project-test.js (4)
29-43: Fix typo in function name.The function name
creatOrgRepohas a typo - it should becreateOrgRepo.-const creatOrgRepo = (propsOverride = {}) => { +const createOrgRepo = (propsOverride = {}) => {Make sure to update all references to this function throughout the test file.
117-118: Update references to the renamed function.If you update the function name from
creatOrgRepotocreateOrgRepo, make sure to update this reference as well.- const orgGithubRepos = [1, 2, 3].map(() => creatOrgRepo()); + const orgGithubRepos = [1, 2, 3].map(() => createOrgRepo());
489-490: Update references to the renamed function.If you update the function name from
creatOrgRepotocreateOrgRepo, make sure to update this reference as well.- creatOrgRepo({ + createOrgRepo({
307-309: Remove redundant conditional check.The conditional check
if (this.hasAttemptedCreatingRepo)is redundant becausethis.set('hasAttemptedCreatingRepo', true)is called unconditionally before the Response is returned.- if (this.hasAttemptedCreatingRepo) { assert.strictEqual(this.notifyService.errorMsg, message()); - }app/components/project-settings/integrations/github-project/index.ts (1)
195-204: Consider consolidating duplicate error handling conditions.The code checks for
errorDetail === 'Github not integrated'twice (lines 196 and 203) with slightly different behavior. Consider consolidating these conditions for better maintainability.- // If Repos exist and error is not integrated, then something went wrong with the integration - if (errorDetail === 'Github not integrated' && this.hasGitHubProject) { - this.currentGithubRepo = null; - this.reconnect = true; - - return; - } - - if (errorDetail === 'Github not integrated') { - this.currentGithubRepo = null; - - return; - } + if (errorDetail === 'Github not integrated') { + this.currentGithubRepo = null; + + if (this.hasGitHubProject) { + this.reconnect = true; + } + + return; + }tests/integration/components/project-settings/integrations/service-now-test.js (1)
115-117: Use a regular expression literal instead of the RegExp constructor.For better readability and to avoid unnecessary escaping, use a regex literal rather than the RegExp constructor.
- .hasAttribute( - 'href', - new RegExp('organization/settings/integrations', 'i') - ); + .hasAttribute( + 'href', + /organization\/settings\/integrations/i + );🧰 Tools
🪛 Biome (1.9.4)
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
tests/integration/components/project-settings/integrations/jira-project-test.js (4)
137-137: Use a regex literal instead of the RegExp constructor.
The static analysis tool suggests using a literal for better readability and maintainability.- new RegExp('organization/settings/integrations', 'i') + /organization\/settings\/integrations/i🧰 Tools
🪛 Biome (1.9.4)
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
141-141: Correct the grammar in the test description.
“atleast” should be “at least.”- test('it renders select button with atleast one JIRA project', ... + test('it renders select button with at least one JIRA project', ...
388-389: Specify a concrete assertion count.
assert.expect()with no argument won’t validate the exact number of assertions, which can mask missing or extra assertions.-test('it deletes selected project when delete trigger is clicked', async function (assert) { - assert.expect(); +test('it deletes selected project when delete trigger is clicked', async function (assert) { + assert.expect(10); // Use the correct count of assertions
462-464: Specify a concrete assertion count.
Same reasoning—passing no argument toassert.expect()doesn't help validate the test thoroughly.-test('it edits the project when a new repo is selected', async function (assert) { - assert.expect(); +test('it edits the project when a new repo is selected', async function (assert) { + assert.expect(12); // Use the correct count of assertionsapp/components/organization/integrations/service-now/index.hbs (1)
29-30: Clarify the form usage or remove it.
Since there’s no submit handling, consider removing the<form>element or adding a correspondingonSubmitaction to make it explicit.-<form class='mt-2' {{style maxWidth='300px'}}> +<div class='mt-2' {{style maxWidth='300px'}}>app/components/organization/integrations/service-now/index.ts (1)
185-185: Adopt optional chaining for cleaner checks.
You can avoid potential undefined references by using an optional chain.- if (changeset.errors && changeset.errors[0].validation) { + if (changeset.errors?.[0]?.validation) {🧰 Tools
🪛 Biome (1.9.4)
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/integration/components/organization/integrations/service-now-test.js (1)
176-176: Missing assertion count in expect() method.The
assert.expect()call is missing the expected assertion count parameter. This could lead to tests that pass even if some assertions are skipped.- assert.expect(); + assert.expect(6); // Update this number to match the actual number of assertions in this test
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/images/service-now.pngis excluded by!**/*.png
📒 Files selected for processing (84)
app/adapters/servicenow-config.ts(1 hunks)app/components/github-account/index.hbs(0 hunks)app/components/github-account/index.scss(0 hunks)app/components/jira-account/index.hbs(0 hunks)app/components/jira-account/index.scss(0 hunks)app/components/marketplace/integration-list/index.ts(1 hunks)app/components/organization/integrations/github-account/index.hbs(1 hunks)app/components/organization/integrations/github-account/index.ts(4 hunks)app/components/organization/integrations/index.hbs(1 hunks)app/components/organization/integrations/index.scss(1 hunks)app/components/organization/integrations/index.ts(1 hunks)app/components/organization/integrations/integrated-ui/index.hbs(1 hunks)app/components/organization/integrations/integrated-ui/index.scss(1 hunks)app/components/organization/integrations/integrated-ui/index.ts(1 hunks)app/components/organization/integrations/jira-account/index.hbs(1 hunks)app/components/organization/integrations/jira-account/index.ts(5 hunks)app/components/organization/integrations/service-now/index.hbs(1 hunks)app/components/organization/integrations/service-now/index.ts(1 hunks)app/components/organization/integrations/service-now/validator.ts(1 hunks)app/components/organization/settings-wrapper/index.ts(1 hunks)app/components/organization/settings/index.hbs(0 hunks)app/components/organization/settings/index.ts(1 hunks)app/components/project-settings/general-settings/github-project/index.hbs(0 hunks)app/components/project-settings/general-settings/index.hbs(1 hunks)app/components/project-settings/general-settings/jira-project/index.hbs(0 hunks)app/components/project-settings/general-settings/jira-project/index.scss(0 hunks)app/components/project-settings/header/index.ts(1 hunks)app/components/project-settings/integrations/config-drawer/index.hbs(1 hunks)app/components/project-settings/integrations/config-drawer/index.scss(1 hunks)app/components/project-settings/integrations/config-drawer/index.ts(1 hunks)app/components/project-settings/integrations/github-project/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/index.ts(9 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.hbs(1 hunks)app/components/project-settings/integrations/github-project/selected-repo/index.ts(1 hunks)app/components/project-settings/integrations/index.hbs(1 hunks)app/components/project-settings/integrations/index.scss(1 hunks)app/components/project-settings/integrations/index.ts(1 hunks)app/components/project-settings/integrations/integrated-header/index.hbs(1 hunks)app/components/project-settings/integrations/integrated-header/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/index.ts(10 hunks)app/components/project-settings/integrations/jira-project/no-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/no-project/index.ts(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.hbs(1 hunks)app/components/project-settings/integrations/jira-project/selected-project/index.ts(1 hunks)app/components/project-settings/integrations/loading-ui/index.hbs(1 hunks)app/components/project-settings/integrations/loading-ui/index.ts(1 hunks)app/components/project-settings/integrations/risk-threshold/index.hbs(1 hunks)app/components/project-settings/integrations/risk-threshold/index.scss(1 hunks)app/components/project-settings/integrations/risk-threshold/index.ts(1 hunks)app/components/project-settings/integrations/service-now/index.hbs(1 hunks)app/components/project-settings/integrations/service-now/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/controllers/authenticated/dashboard/organization-settings/service-account.ts(1 hunks)app/controllers/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/enums.ts(1 hunks)app/models/servicenow-config.ts(1 hunks)app/router.ts(3 hunks)app/routes/authenticated/dashboard/organization-settings/index.ts(1 hunks)app/routes/authenticated/dashboard/organization-settings/integrations.ts(1 hunks)app/routes/authenticated/dashboard/project/settings/integrations.ts(1 hunks)app/styles/_component-variables.scss(2 hunks)app/templates/authenticated/dashboard/organization-settings/integrations.hbs(1 hunks)app/templates/authenticated/dashboard/project/settings/integrations.hbs(1 hunks)config/environment.js(2 hunks)mirage/factories/github-integration.ts(0 hunks)mirage/factories/jira-integration.ts(0 hunks)mirage/factories/jira-repo.ts(1 hunks)mirage/factories/servicenow-config.ts(1 hunks)mirage/scenarios/default.js(0 hunks)tests/integration/components/github-account-test.js(0 hunks)tests/integration/components/jira-account-test.js(0 hunks)tests/integration/components/organization/integrations/github-account-test.js(1 hunks)tests/integration/components/organization/integrations/jira-account-test.js(1 hunks)tests/integration/components/organization/integrations/service-now-test.js(1 hunks)tests/integration/components/project-settings/general-settings/github-project-test.js(0 hunks)tests/integration/components/project-settings/general-settings/jira-project-test.js(0 hunks)tests/integration/components/project-settings/integrations/github-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/jira-project-test.js(1 hunks)tests/integration/components/project-settings/integrations/service-now-test.js(1 hunks)translations/en.json(8 hunks)translations/ja.json(8 hunks)types/irene/index.d.ts(1 hunks)
💤 Files with no reviewable changes (15)
- mirage/scenarios/default.js
- app/components/organization/settings/index.hbs
- app/components/project-settings/general-settings/github-project/index.hbs
- app/components/project-settings/general-settings/jira-project/index.hbs
- mirage/factories/github-integration.ts
- app/components/project-settings/general-settings/jira-project/index.scss
- mirage/factories/jira-integration.ts
- tests/integration/components/jira-account-test.js
- tests/integration/components/project-settings/general-settings/jira-project-test.js
- app/components/github-account/index.hbs
- app/components/jira-account/index.scss
- app/components/github-account/index.scss
- tests/integration/components/github-account-test.js
- app/components/jira-account/index.hbs
- tests/integration/components/project-settings/general-settings/github-project-test.js
🚧 Files skipped from review as they are similar to previous changes (53)
- app/components/organization/integrations/index.scss
- app/components/project-settings/integrations/index.scss
- app/templates/authenticated/dashboard/organization-settings/integrations.hbs
- types/irene/index.d.ts
- app/components/project-settings/integrations/jira-project/no-project/index.hbs
- app/templates/authenticated/dashboard/project/settings/integrations.hbs
- app/components/organization/integrations/index.ts
- app/routes/authenticated/dashboard/project/settings/integrations.ts
- app/components/project-settings/integrations/risk-threshold/index.scss
- app/components/project-settings/integrations/github-project/selected-repo/index.hbs
- app/components/project-settings/integrations/risk-threshold/index.hbs
- app/components/project-settings/general-settings/index.hbs
- app/components/project-settings/integrations/loading-ui/index.hbs
- app/components/organization/integrations/integrated-ui/index.scss
- config/environment.js
- app/components/organization/integrations/integrated-ui/index.hbs
- app/components/project-settings/integrations/jira-project/no-project/index.ts
- app/enums.ts
- app/components/project-settings/integrations/jira-project/selected-project/index.ts
- app/controllers/authenticated/dashboard/organization-settings/service-account.ts
- app/components/organization/integrations/index.hbs
- app/routes/authenticated/dashboard/organization-settings/integrations.ts
- app/components/organization/integrations/service-now/validator.ts
- app/components/project-settings/integrations/index.hbs
- app/components/marketplace/integration-list/index.ts
- app/router.ts
- mirage/factories/jira-repo.ts
- app/components/project-settings/header/index.ts
- app/models/servicenow-config.ts
- app/components/organization/integrations/integrated-ui/index.ts
- app/components/organization/integrations/jira-account/index.ts
- app/styles/_component-variables.scss
- app/components/organization/integrations/github-account/index.ts
- app/components/organization/integrations/github-account/index.hbs
- app/components/project-settings/integrations/jira-project/selected-project/index.hbs
- app/components/project-settings/integrations/index.ts
- app/components/project-settings/integrations/github-project/selected-repo/index.ts
- app/components/organization/settings-wrapper/index.ts
- app/controllers/authenticated/dashboard/organization-settings/integrations.ts
- app/components/organization/settings/index.ts
- app/routes/authenticated/dashboard/organization-settings/index.ts
- app/components/project-settings/integrations/integrated-header/index.hbs
- app/components/project-settings/integrations/config-drawer/index.hbs
- mirage/factories/servicenow-config.ts
- app/components/project-settings/integrations/risk-threshold/index.ts
- translations/ja.json
- app/components/project-settings/integrations/config-drawer/index.scss
- app/controllers/authenticated/dashboard/organization-settings/index.ts
- app/components/project-settings/integrations/loading-ui/index.ts
- app/components/project-settings/integrations/integrated-header/index.ts
- app/components/project-settings/integrations/jira-project/index.ts
- app/components/organization/integrations/jira-account/index.hbs
- app/components/project-settings/integrations/config-drawer/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
tests/integration/components/project-settings/integrations/jira-project-test.js
[error] 137-137: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
tests/integration/components/project-settings/integrations/github-project-test.js
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
app/components/organization/integrations/service-now/index.ts
[error] 185-185: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
tests/integration/components/project-settings/integrations/service-now-test.js
[error] 116-116: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.
(lint/complexity/useRegexLiterals)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Publish to Cloudflare Pages
- GitHub Check: Setup & Build Application
- GitHub Check: Cloudflare Pages
🔇 Additional comments (37)
tests/integration/components/project-settings/integrations/github-project-test.js (2)
197-200: Use regex literal instead of RegExp constructor.Following best practices, use a regex literal instead of the RegExp constructor for better readability and to avoid unnecessary string escaping issues.
- .hasAttribute( - 'href', - new RegExp('organization/settings/integrations', 'i') - ); + .hasAttribute( + 'href', + /organization\/settings\/integrations/i + );🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
71-568: Well-structured test suite with comprehensive coverage.The test suite is well-organized, with comprehensive coverage of different scenarios for the GitHub project integration. The tests cover initialization, rendering, user interactions, error handling, and CRUD operations.
The use of helper functions like
assertIntegratedHeaderActionItemsandgetAnalysisRisklabelimproves code reuse and readability. The mock services and server responses are appropriately set up to test different code paths.🧰 Tools
🪛 Biome (1.9.4)
[error] 199-199: Use a regular expression literal instead of the RegExp constructor.
Regular expression literals avoid some escaping required in a string literal, and are easier to analyze statically.
Safe fix: Use a literal notation instead.(lint/complexity/useRegexLiterals)
app/components/project-settings/integrations/github-project/index.ts (2)
137-139: LGTM! Good addition of computed property.The
showSelectPrjCTAcomputed property makes the template rendering logic cleaner by encapsulating the condition for showing the project selection call-to-action.
74-92: Improved UI logic with informative computed properties.The addition of these computed properties improves the component's readability by centralizing UI logic that determines when to display different text and UI elements based on the integration state.
app/controllers/authenticated/dashboard/project/settings/integrations.ts (1)
13-32: Well-structured breadcrumbs implementation.The breadcrumbs implementation follows good practices by including:
- Dynamic title that combines the integration label with project name
- Proper route hierarchy with parent/sibling relationships
- Support for internationalization through the intl service
This will enhance user navigation throughout the application.
app/adapters/servicenow-config.ts (3)
13-24: Add error handling to the updateRecord method.The updateRecord method doesn't include any error handling, which could lead to unhandled promise rejections.
updateRecord<K extends keyof ModelRegistry>( _: Store, _type: ModelSchema<K>, snapshot: Snapshot<K> ) { const { riskThreshold } = snapshot.record as ServiceNowConfigModel; const url = this._buildURL(); - return this.ajax(url, 'POST', { - data: { risk_threshold: riskThreshold }, - }); + return this.ajax(url, 'POST', { + data: { risk_threshold: riskThreshold }, + }).catch(error => { + console.error('Error updating ServiceNow config:', error); + throw error; + }); }
8-11: LGTM! Well-structured URL building method.The
_buildURL()method clearly constructs the ServiceNow API endpoint by leveraging the base URL building functionality from the parent adapter.
26-28: LGTM! Proper namespacing method.The
setNestedUrlNamespacemethod appropriately scopes API requests to a specific project, allowing for proper isolation of ServiceNow configurations per project.tests/integration/components/project-settings/integrations/service-now-test.js (3)
54-83: Well-structured test setup with proper isolation.The test setup properly:
- Uses setupRenderingTest, setupMirage, and setupIntl hooks
- Registers a NotificationsStub service to avoid side effects
- Creates test data through Mirage server
- Sets up proper test context properties
This ensures tests run in isolation and with controlled dependencies.
206-278: Comprehensive threshold selection test with proper assertions.This test thoroughly validates the ServiceNow integration threshold selection workflow:
- Mocks the initial API response showing no integration
- Handles the POST request for creating a new configuration
- Verifies UI elements before and after selection
- Checks notification messages
- Asserts the correct risk level is displayed
Great job on covering this critical user journey comprehensively.
280-341: Well-implemented deletion test.This test properly validates the delete functionality by:
- Setting up the initial ServiceNow config in Mirage
- Testing the UI elements before deletion
- Simulating the user flow of clicking the delete action
- Verifying the confirmation modal appears
- Confirming the deletion and checking the resulting UI state
The assertion count (expect(12)) helps ensure all important aspects are verified.
translations/en.json (4)
200-204: ServiceNow confirmation messages correctly added.The new confirmation messages for removing and revoking ServiceNow configuration have been properly added to the confirmation box object, following the existing pattern used for other integrations.
609-612: ServiceNow integration strings properly implemented.The integration strings follow the same pattern as other integration types like GitHub and JIRA, maintaining consistency in the UI messaging.
1045-1045: ServiceNow risk threshold selection string properly added.The new translation string for selecting ServiceNow risk threshold has been appropriately added to the otherTemplates section.
1455-1468: Complete ServiceNow integration translation section added.A well-structured ServiceNow section has been added with all necessary UI strings for the integration, including titles, labels, and status messages. The section follows the same pattern as other integration sections in the file.
tests/integration/components/organization/integrations/github-account-test.js (1)
1-260: Well-structured test file with comprehensive test coverage.This test file thoroughly covers all aspects of the GitHub account integration component:
- Tests for non-integrated, integrated, and reconnect states
- Proper testing of DOM elements and their states
- Comprehensive error handling tests with different scenarios
- Clear assertions and good test organization with the test.each pattern for different scenarios
- Proper cleanup in tests that modify the window location
app/components/project-settings/integrations/service-now/index.ts (2)
1-22: Well-structured component with appropriate service injections.The component correctly imports and uses Ember and Glimmer functionality, and properly defines services and interfaces.
24-210: ServiceNow integration component properly implements required functionality.The component includes:
- Well-managed state with tracked properties
- Comprehensive error handling in tasks
- Clear action methods
- Proper integration with the store
- Good handling of conditional UI states through getters
The implementation follows Ember best practices and handles various edge cases correctly.
tests/integration/components/organization/integrations/jira-account-test.js (1)
1-398: Well-implemented JIRA integration tests.The test file includes comprehensive coverage of the JIRA account integration component:
- Tests for non-integrated and integrated states
- Input validation testing
- Success and error handling for integration and disconnection
- Proper DOM assertions for UI elements
- Good mocking of server responses
The tests follow a consistent pattern with the GitHub integration tests, maintaining a unified approach to testing integrations.
Also applies to: 403-442
app/components/project-settings/integrations/github-project/index.hbs (3)
1-52: Clean structure with good conditional handling for different states.The component's layout follows a clear hierarchical structure with appropriate conditional rendering for different states (loading, repo selected, CTA, or no integration). The use of data test attributes for each element is excellent for testing.
54-114: Well-structured configuration drawer with all necessary UI elements.The configuration drawer implementation provides a clear interface for selecting GitHub repositories and risk thresholds. The conditional rendering based on
this.githubReposavailability ensures a good user experience.
116-122: Consistent confirmation box implementation for delete action.The confirmation box implementation follows the pattern used across the application, making for a consistent user experience.
app/components/project-settings/integrations/service-now/index.hbs (3)
1-75: Well-structured ServiceNow integration component with appropriate conditional states.The component properly handles various states (loading, configured, no access, no integration) with appropriate UI elements for each case. Good use of shared components like
ProjectSettings::Integrations::IntegratedHeaderandProjectSettings::Integrations::LoadingUifor consistency.
77-117: Config drawer implementation matches the pattern used in other integrations.The configuration drawer for risk threshold selection follows the same pattern as GitHub and JIRA integrations, ensuring consistent UX across the application.
119-125: Confirm box for deletion follows the established pattern.The confirmation dialog implementation for deleting ServiceNow configuration aligns with other integration components, maintaining UI consistency.
app/components/project-settings/integrations/jira-project/index.hbs (3)
1-64: Clean implementation with appropriate conditional states for JIRA integration.The component effectively handles different states with conditional rendering. Good use of shared components for consistency with other integration UIs.
66-138: Well-structured configuration drawer with note about JIRA account selection.The configuration drawer includes all necessary UI elements for repository and threshold selection. The added note explaining the JIRA account selection process provides helpful context for users.
140-146: Consistent confirmation box implementation.The confirmation dialog implementation follows the established pattern used in other integration components.
tests/integration/components/organization/integrations/service-now-test.js (9)
192-192: Missing assertion count in expect() method.Similar to the previous test, this
assert.expect()call is missing the expected assertion count parameter.- assert.expect(); + assert.expect(4); // Update this number to match the actual number of assertions in this test
39-103: Well-structured assertion helper for non-integrated UI state.The
assertNonIntegratedUIhelper function provides comprehensive assertions for the expected UI elements when ServiceNow is not integrated. This promotes test maintainability and readability.
105-158: Comprehensive assertion helper for integrated UI state.The
assertIntegratedUIhelper function properly verifies all expected UI elements when ServiceNow is integrated, ensuring the component displays the correct information.
175-189: Good test coverage for non-integrated state.This test effectively verifies the UI when ServiceNow is not integrated, including checking that the component renders the expected title and non-integrated UI elements.
191-212: Incomplete test for integrated state.The test creates and mocks the integrated state but doesn't call or verify against the
assertIntegratedUIhelper that was defined earlier.
207-207: Good use of helper function for assertion validation.Using the
assertIntegratedUIhelper function ensures consistent verification of UI elements across tests.
214-295: Comprehensive test for disconnecting ServiceNow with both success and failure cases.This test thoroughly covers the disconnect functionality, testing both successful and failed disconnections. It verifies the UI state changes, modal behavior, and proper notification messages.
297-318: Good validation testing for required inputs.This test effectively verifies that the component validates inputs before submission, checking for appropriate error states when validation fails.
320-455: Thorough integration test with multiple scenarios.The test covers both successful integration and different failure cases, verifying appropriate error messages and UI state retention. Good use of test.each to parameterize the test for different scenarios.
| assert.dom('[data-test-jiraAccount-hostInput]').doesNotExist(); | ||
| assert.dom('[data-test-jiraAccount-hostInput]').doesNotExist(); | ||
| assert.dom('[data-test-jiraAccount-usernameInput]').doesNotExist(); | ||
| assert.dom('[data-test-jiraAccount-apiKeyInput]').doesNotExist(); |
There was a problem hiding this comment.
Duplicate assertion found.
There's a duplicate assertion for [data-test-jiraAccount-hostInput]. The second assertion should likely be testing a different element.
assert.dom('[data-test-jiraAccount-hostInput]').doesNotExist();
-assert.dom('[data-test-jiraAccount-hostInput]').doesNotExist();
+assert.dom('[data-test-jiraAccount-usernameInput]').doesNotExist();
assert.dom('[data-test-jiraAccount-usernameInput]').doesNotExist();
assert.dom('[data-test-jiraAccount-apiKeyInput]').doesNotExist();Committable suggestion skipped: line range outside the PR's diff.
| @onDisconnectClick={{this.openRevokeGithubConfirmBox}} | ||
| /> | ||
|
|
||
| {{else}} |
| {{#if @loading}} | ||
| <AkSkeleton @width='94px' @height='38px' /> | ||
|
|
||
| {{else}} |
| @@ -0,0 +1,6 @@ | |||
| .org-integration-settings-container { | |||
| background-color: var(--organization-settings-container-background-color); | |||
There was a problem hiding this comment.
can we make and use another variable?
--organization-integrations-background-color
| data-test-orgIntegrations-serviceNow-integratedHeader | ||
| /> | ||
|
|
||
| {{else}} |
| border: 1px solid var(--project-settings-general-settings-root-border-color); | ||
| border-top: 0em; | ||
| background-color: var( | ||
| --project-settings-general-settings-root-background-color |
There was a problem hiding this comment.
use variables of 'project-settings-integrations'
| data-test-projectSettings-generalSettings-root | ||
| > | ||
|
|
||
| <AkStack @width='full' class='px-3'> |
No description provided.