-
Notifications
You must be signed in to change notification settings - Fork 13
[JBEAP-29211] Add filter to Configurable HTTP Server Mechanism Factory #127
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
base: main
Are you sure you want to change the base?
Conversation
| flip(formId: string, attributeName: string, value: boolean): Chainable<void>; | ||
| /** | ||
| * Set text value to form input. | ||
| * Set text value to form input. The id of form filed is concatenated like: cy.get("#" + configurationFormId + "-" + attributeName + "-editing"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get how is the element ID concatenate. I have to look at the implementation to understand what I have use for formId and attributeName. So i just the important part into description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, perhaps
@param formId - The ID of section which contain form inputs. This parameter is appended with -editing to obtain full ID is more clear. On the other hand most of function present in this class do the same, without this explanation, introducing Inconsistency.
However I don't have strong opinion about this
...lytron/test-configuration-subsystem-elytron-configurable-http-server-mechanism-factory.cy.ts
Outdated
Show resolved
Hide resolved
...lytron/test-configuration-subsystem-elytron-configurable-http-server-mechanism-factory.cy.ts
Outdated
Show resolved
Hide resolved
...lytron/test-configuration-subsystem-elytron-configurable-http-server-mechanism-factory.cy.ts
Show resolved
Hide resolved
| }; | ||
|
|
||
| const addButtonInForm = "hal-modal > div > div > div.modal-footer"; | ||
| const filtrsButtonForConfigurableHttpServer = "#hal-uid-1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this random number changes between version, ID shouldn't be used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the ID. Or do you mean something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the ID suffix is random number and shouldnt be relied on
| cy.addInTable(httpServerMechanismFactory + "-table"); | ||
| cy.text(factoryForm.id, factoryForm.fieldName.name, factoryForm.fieldName.value); | ||
| cy.text(factoryForm.id, factoryForm.fieldFactory.name, factoryForm.fieldFactory.value); | ||
| cy.addSingletonResource(addButtonInForm); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| cy.addSingletonResource(addButtonInForm); | |
| cy.confirmAddResourceWizard(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. updated.
| const filterForm = { | ||
| id: httpServerMechanismFactoryFiltrs + "-add", | ||
| fieldEnabled: { | ||
| id: "#" + httpServerMechanismFactoryFiltrs + "-add-editing > div:nth-child(2)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try avoiding nth-child method for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like but here it nonstandard check to verify the text. The element itself doesn't have any specific ID to be able use as a selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we use something like here:
Line 130 in c49dc9b
| cy.formInput(configurationFormId, "static-ejb-discovery") |
laDok8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few changes required
| cy.navigateToSpecificChannel(managementEndpoint, channels.url.name); | ||
| cy.editForm(channelForm); | ||
| cy.get(".tm-tag-remove").click() | ||
| cy.get(".tm-tag-remove").click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't disagree with this changes, but it's unnecessarily increasing changed files/PR size here
Thanks for submitting your Pull Request!
Please delete this text, and add a link to the public Jira issue and/or Berg issue solved by this PR, if available.
If this PR is not for the 'main' branch you must add a link to the equivalent change in 'main'.
Remember to use the Jira issue ID or Berg issue number in the PR title and any commits, if available.
Keep the title/summary of PR and any commits descriptive, so that the repository history is human readable.