fix: Blocking disconnection of the only connected auth method in admin settings #40150
Conversation
WalkthroughThe changes update Cypress test files by adding an extra authentication tag to the existing settings tag. In the application code, a helper function now verifies that Google OAuth is enabled before returning a positive authentication check. Additionally, the Admin Settings form component removes redundant state management, directly relying on the current state for form login. No exported entities were altered. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as SettingsForm
participant H as adminSettingsHelpers
U->>S: Initiate Save Operation
S->>H: Call saveAllowed(settings)
H->>H: Evaluate conditions (including Google OAuth enabled check)
H-->>S: Return Boolean result
S->>S: Proceed with save/disconnect logic based on result
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14318957985. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js (1)
23-23: 💡 Verification agent🧩 Analysis chain
Consider replacing cy.wait() with more robust waiting strategies.
The test contains multiple hardcoded waits which can make tests flaky. Replace with cy.waitUntil() with appropriate conditions or use Cypress's built-in retry-ability.
-cy.wait(2000); +cy.waitUntil(() => cy.get(adminSettings.googleForm).should('be.visible'));Also applies to: 46-47, 49-49, 58-59, 80-80, 97-98, 101-102
🏁 Script executed:
#!/bin/bash # Check usage of cy.wait() in Google authentication tests rg -n "cy\.wait\(" app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js # Check for better waiting patterns that could be adopted rg -n "cy\.waitUntil" app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.jsLength of output: 597
Replace all hardcoded waits with condition-based waiting for stability
The test file still contains several hardcoded
cy.wait()calls (e.g., at lines 23, 46, 49, 58, 62, 80, 97, and 101) that can lead to flaky tests. While some instances have already been updated to usecy.waitUntil(), the remaining hardcoded waits should be replaced with robust waiting strategies tailored to the element or state that confirms test progress. For example, as shown below, instead of using a fixed delay:-cy.wait(2000); +cy.waitUntil(() => cy.get(adminSettings.googleForm).should('be.visible'));Action items:
- Line 23, 46, 49, 80: Replace
cy.wait(2000)with a correspondingcy.waitUntil()checking for the relevant element’s visibility.- Line 58 & 97: Instead of
cy.wait(1000), wait for the appropriate DOM condition to be met.- Line 62 & 101: Replace
cy.wait(500)with an equivalent conditional wait.- Consistency Check: Verify that each replacement uses the correct selector (e.g.,
adminSettings.googleFormorhomePage.profileMenu) that aligns with the expected state for that part of the test.This change will exploit Cypress’s built-in retry-ability, reduce test flakiness, and make the tests easier to maintain.
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js (1)
16-16: 💡 Verification agent🧩 Analysis chain
Replace long cy.wait() calls with more deterministic waits.
The test contains long waits (up to 120 seconds) which make tests slow and potentially flaky. Consider implementing more robust waiting strategies.
-cy.wait(2000); +cy.waitUntil(() => cy.get(adminSettings.formSignupDisabled).should('be.visible')); -cy.wait(120000); +cy.waitUntil(() => cy.contains("GitHub authentication").should('be.visible'), { + timeout: 120000, + interval: 2000 +});Also applies to: 20-20, 79-80, 130-131
🏁 Script executed:
#!/bin/bash # Check for long wait times in test files rg -n "cy\.wait\(1[0-9]{4,}" app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js # Check for existing waitUntil patterns rg -n "cy\.waitUntil" app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.jsLength of output: 749
Action: Replace remaining non-deterministic waits with proper waitUntil calls
Please update the following in your test file:
Line 16 (cy.wait(2000))
Replace this with a deterministic wait (e.g., waiting for the element that indicates the form’s readiness). For instance:-cy.wait(2000); +cy.waitUntil(() => cy.get(adminSettings.formSignupDisabled).should('be.visible'));Lines 80 and 130 (cy.wait(120000))
These long fixed waits still exist. Replace them with a more robust condition that waits for the expected UI change. For example:-cy.wait(120000); +cy.waitUntil(() => cy.contains("GitHub authentication").should('be.visible'), { + timeout: 120000, + interval: 2000 +});Alternatively, if another element (such as the already-used
homePage.profileMenu) better reflects the state of readiness, ensure consistency across the test.Using precise conditions instead of arbitrary wait times helps avoid slow and flaky tests. Please adjust the selectors to match the intended behavior.
🧹 Nitpick comments (1)
app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js (1)
13-14:❓ Verification inconclusive
Replace string selectors with data- attributes.*
Using string selectors like ".admin-settings-menu-option" should be avoided according to coding guidelines. Consider defining these in the locators file and/or using data attributes.
-cy.get(".admin-settings-menu-option").should("be.visible"); -cy.get(".admin-settings-menu-option").click(); +cy.get(adminSettings.adminSettingsMenu).should("be.visible"); +cy.get(adminSettings.adminSettingsMenu).click();Also applies to: 36-37
🏁 Script executed:
#!/bin/bash # Check for string selectors in test files rg -n "cy\.get\(\"\." app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js # Check for existing locator definitions rg -n "adminSettingsMenu" app/client/cypress/locators/AdminsSettings.jsLength of output: 542
Attention: Update Test Selectors for Admin Settings
The tests currently use the hard-coded string selector
".admin-settings-menu-option"on lines 13–14, 36–37, and 70–71. Please refactor these to use the locator defined asadminSettings.adminSettingsMenu(as shown in the diff snippet). However, our search didn't find any occurrence ofadminSettingsMenuin the expected locator file (app/client/cypress/locators/AdminsSettings.js). Could you please verify that this locator is defined and correctly exported in that file (or update it accordingly)? Once confirmed, update the tests as follows:-cy.get(".admin-settings-menu-option").should("be.visible"); -cy.get(".admin-settings-menu-option").click(); +cy.get(adminSettings.adminSettingsMenu).should("be.visible"); +cy.get(adminSettings.adminSettingsMenu).click();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js(1 hunks)app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js(1 hunks)app/client/src/ce/utils/adminSettingsHelpers.ts(1 hunks)app/client/src/pages/AdminSettings/SettingsForm.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`app/client/cypress/**/**.*`: Review the following e2e test code written using the Cypress test library. Ensure that: - Follow best practices for Cypress code and e2e automation. ...
app/client/cypress/**/**.*: Review the following e2e test code written using the Cypress test library. Ensure that:
- Follow best practices for Cypress code and e2e automation.
- Avoid using cy.wait in code.
- Avoid using cy.pause in code.
- Avoid using agHelper.sleep().
- Use locator variables for locators and do not use plain strings.
- Use data-* attributes for selectors.
- Avoid Xpaths, Attributes and CSS path.
- Avoid selectors like .btn.submit or button[type=submit].
- Perform logins via API with LoginFromAPI.
- Perform logout via API with LogOutviaAPI.
- Perform signup via API with SignupFromAPI.
- Avoid using it.only.
- Avoid using after and aftereach in test cases.
- Use multiple assertions for expect statements.
- Avoid using strings for assertions.
- Do not use duplicate filenames even with different paths.
- Avoid using agHelper.Sleep, this.Sleep in any file in code.
app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.jsapp/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js
🧬 Code Definitions (1)
app/client/src/pages/AdminSettings/SettingsForm.tsx (1)
app/client/src/ce/utils/adminSettingsHelpers.ts (1)
saveAllowed(13-37)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / rts-build / build
- GitHub Check: client-lint / client-lint
- GitHub Check: client-unit-tests / client-unit-tests
- GitHub Check: client-check-cyclic-deps / check-cyclic-dependencies
- GitHub Check: client-build / client-build
- GitHub Check: client-prettier / prettier-check
🔇 Additional comments (6)
app/client/cypress/e2e/Regression/ClientSide/Google/EnableGoogle_spec.js (1)
7-8: Tag addition for better test categorization.Adding the Authentication tag helps organize tests by functional area, improving test management and selective test runs.
app/client/cypress/e2e/Regression/ClientSide/FormLogin/EnableFormLogin_spec.js (1)
6-7: Tag addition for better test categorization.Adding the Authentication tag improves test organization by functional area, consistent with other authentication test files.
app/client/src/ce/utils/adminSettingsHelpers.ts (1)
26-28: Added check for Google OAuth enabled status to prevent disconnection of the only auth method.This change ensures that Google authentication is only considered valid if both the client ID is provided AND the OAuth functionality is explicitly enabled. This helps prevent users from disconnecting their only authentication method, which could lock them out.
This modification supports the PR objective to prevent disconnection of the only connected authentication method in admin settings.
app/client/src/pages/AdminSettings/SettingsForm.tsx (3)
1-1: Removed unused useState import after simplifying state management.The code now uses direct state values rather than tracking initial states separately, which is a cleaner approach.
155-156: Simplified state management for form login enabled check.The code now directly uses the current
isFormLoginEnabledvalue instead of comparing against a stored initial state. This reduces complexity and potential for state synchronization issues.
250-251: Updated connected methods count calculation to use current form login state.This change ensures consistent use of the current login state throughout the component rather than relying on stored initial values, improving reliability when determining if disconnection is allowed.
|
Deploy-Preview-URL: https://ce-40150.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/14326381853. |
|
Deploy-Preview-URL: https://ce-40150.dp.appsmith.com |
…n settings (appsmithorg#40150) ## Description Blocking disconnection of the only connected auth method in admin settings to avoid locking out the user. Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /ok-to-test tags="@tag.Settings, @tag.Authentication, @tag.SignIn" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/14326343041> > Commit: 1c158e1 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=14326343041&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Settings, @tag.Authentication, @tag.SignIn` > Spec: > <hr>Tue, 08 Apr 2025 06:31:56 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Updated Google login handling to ensure the system now accurately verifies when Google OAuth is enabled, improving the reliability of authentication. - Enhanced the categorization of test cases related to Form Login and Google authentication, ensuring more precise test execution. - **Refactor** - Streamlined login settings management in the admin interface by removing redundant state management, ensuring consistent and up-to-date behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
Blocking disconnection of the only connected auth method in admin settings to avoid locking out the user.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags="@tag.Settings, @tag.Authentication, @tag.SignIn"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/14326343041
Commit: 1c158e1
Cypress dashboard.
Tags:
@tag.Settings, @tag.Authentication, @tag.SignInSpec:
Tue, 08 Apr 2025 06:31:56 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Bug Fixes
Refactor