Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] Add clickDialogButton methods for FedCM #14072

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

cbiesinger
Copy link
Contributor

@cbiesinger cbiesinger commented Jun 3, 2024

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 7ad44ee

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: w3c-fedid/FedCM#610

Types of changes

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

Checklist

  • I have read the contributing document.
  • [?] My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • 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

    Copy link
    Contributor

    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.

    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

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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]

    -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
    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]

    -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]

    -executeMethod.execute(DriverCommand.CLICK_DIALOG, Map.of("dialogButton", button));
    +private Map<String, Object> 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
    Copy link
    Contributor

    pujagani commented Jun 4, 2024

    Is there a way to test the new APIs added?

    @cbiesinger
    Copy link
    Contributor Author

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

    @cbiesinger cbiesinger force-pushed the clickdialog branch 2 times, most recently from 46f33d8 to 532fab7 Compare June 4, 2024 20:41
    This builds on commit 7ad44ee
    
    The specification for clickdialogbutton is here:
    https://fedidcg.github.io/FedCM/#webdriver-clickdialogbutton
    
    The version that takes an index is specified here:
    w3c-fedid/FedCM#610
    
    Bug SeleniumHQ#12088
    @VietND96 VietND96 changed the title Add clickDialogButton methods for FedCM [java] Add clickDialogButton methods for FedCM Nov 15, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants