Add clickDialogButton methods for FedCM

cbiesinger commented 1 month ago

User description

Thanks for contributing to Selenium! A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines. Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This builds on commit 7ad44eef93a2797fdfa14f05f0ff0bb8c4a2525f

Bug #12088

Motivation and Context

An additional command has been added for FedCM.

The specification for clickdialogbutton is here: https://fedidcg.github.io/FedCM/#webdriver-clickdialogbutton

The version that takes an index is specified here: https://github.com/fedidcg/FedCM/pull/610

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

[x] I have read the contributing document.
[?] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

PR Type

Enhancement

Description

Added new constants for dialog buttons in FederatedCredentialManagementDialog.
Introduced clickButton methods in FederatedCredentialManagementDialog to handle button clicks with and without an account index.
Implemented the clickButton methods in FedCmDialogImpl to execute the corresponding driver commands.

Changes walkthrough 📝

Relevant files

Enhancement

FederatedCredentialManagementDialog.java `Add methods to click dialog buttons in FedCM` java/src/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementDialog.java Added constants for new dialog buttons. Introduced `clickButton` method for clicking buttons without an index. Introduced `clickButton` method for clicking buttons with an account index.	+20/-0
FedCmDialogImpl.java `Implement button click methods in FedCmDialogImpl` java/src/org/openqa/selenium/remote/FedCmDialogImpl.java Implemented `clickButton` method to handle button clicks without an index. Implemented `clickButton` method to handle button clicks with an account index.	+11/-0

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the PR introduces a new feature with a moderate amount of new code, mainly focused on adding button interaction capabilities to the Federated Credential Management dialog. The changes are straightforward and localized to specific methods, making the review process less complex.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The PR lacks null checks or validation for the `button` parameter in the `clickButton` methods. This could lead to potential runtime errors if null or invalid button identifiers are passed.
⚡ Possible issues	Missing Documentation: The PR description mentions that documentation changes might be required, but it's unclear if the documentation has been updated accordingly.
🔒 Security concerns	No

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add validation to ensure the `button` parameter is not null or empty before proceeding ___ Consider adding validation to ensure that the `button` parameter is not null or empty before proceeding with the `clickButton` method. This can help prevent potential runtime errors. [java/src/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementDialog.java [72]](https://github.com/SeleniumHQ/selenium/pull/14072/files#diff-2bf62dbea4f50873dd623f55e0a5f03cc9a40e1d6c01b3dacc4bb0e7b34bcd54R72-R72) ```diff -void clickButton(String button); +void clickButton(String button) { + if (button == null \|\| button.isEmpty()) { + throw new IllegalArgumentException("Button cannot be null or empty"); + } + // existing logic +} ``` Suggestion importance[1-10]: 8 Why: Adding null or empty checks for the `button` parameter in the `clickButton` method is crucial to prevent runtime errors and ensure robustness.	8
Possible issue	Add validation to ensure the `button` parameter is not null or empty and the `index` parameter is non-negative before proceeding ___ Consider adding validation to ensure that the `button` parameter is not null or empty and the `index` parameter is non-negative before proceeding with the `clickButton` method that requires an account index. [java/src/org/openqa/selenium/federatedcredentialmanagement/FederatedCredentialManagementDialog.java [80]](https://github.com/SeleniumHQ/selenium/pull/14072/files#diff-2bf62dbea4f50873dd623f55e0a5f03cc9a40e1d6c01b3dacc4bb0e7b34bcd54R80-R80) ```diff -void clickButton(String button, int index); +void clickButton(String button, int index) { + if (button == null \|\| button.isEmpty()) { + throw new IllegalArgumentException("Button cannot be null or empty"); + } + if (index < 0) { + throw new IllegalArgumentException("Index cannot be negative"); + } + // existing logic +} ``` Suggestion importance[1-10]: 8 Why: Validating both the `button` parameter for non-null or empty values and the `index` parameter for non-negative values is essential for error prevention and data integrity in the method that requires an account index.	8
Maintainability	Extract the map creation logic into a separate method for improved readability and maintainability ___ To improve readability and maintainability, consider extracting the map creation logic into a separate method for the `clickButton` methods. [java/src/org/openqa/selenium/remote/FedCmDialogImpl.java [76]](https://github.com/SeleniumHQ/selenium/pull/14072/files#diff-f3518c3c8c1971772ae6b334d272314f5dc27474f516c54655ab8dae59f69aedR76-R76) ```diff -executeMethod.execute(DriverCommand.CLICK_DIALOG, Map.of("dialogButton", button)); +private Map createButtonMap(String button) { + return Map.of("dialogButton", button); +} +@Override +public void clickButton(String button) { + executeMethod.execute(DriverCommand.CLICK_DIALOG, createButtonMap(button)); +} + ``` Suggestion importance[1-10]: 6 Why: Extracting the map creation logic into a separate method enhances readability and maintainability, although it's a moderate improvement and not critical for functionality.	6

pujagani commented 1 month ago

Is there a way to test the new APIs added?

cbiesinger commented 4 weeks ago

OK, I added a test. Note that it will fail until PR #14070 gets merged.

SeleniumHQ / selenium