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

Update Faces 2.3 Bean Validation Application to Address Challenge 1715 #1768

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

volosied
Copy link
Contributor

fixes issue reported in #1715

@volosied
Copy link
Contributor Author

volosied commented Dec 13, 2022

Please hold off merging. I tested this a while back, but would like to verify again.

Edit: All good. Manual tests show it works as intended (error message when passwords don't match - for both Issue4313 & Issue4083) on Mojarra 4.0.0 and MyFaces' latest snapshot.

@volosied
Copy link
Contributor Author

MyFaces still fails since we append the component id (resulting in form:j_id_g: Password fields must match), and the test checks for an exact match in the message. We may need to update the MyFaces impl to pass the test unless the test should be updated to check for contains...? I will follow up on this.

@volosied
Copy link
Contributor Author

volosied commented Jan 10, 2023

@BalusC @arjantijms

Would the community allow another update to the tests? MyFaces prefixes the message with the form id as indicated in my comment above (form:j_id_g: Password fields must match). This fails due to assertEquals check. Proposed fix would be to check if the message is contained instead.

MyFaces tries to use the label first, but, if nonexistent, it defaults to the id:
https://github.com/apache/myfaces/blob/e9fe59f96410f31a7f5c0fbd6838c1a22683a691/impl/src/main/java/org/apache/myfaces/util/MessageUtils.java#L760

Exception handling occurs here: https://github.com/apache/myfaces/blob/e9fe59f96410f31a7f5c0fbd6838c1a22683a691/impl/src/main/java/org/apache/myfaces/component/validate/WholeBeanValidator.java#L145-L149

Currently the TCK tests here use assertEquals.

assertEquals("Password fields must match", page1.getElementById("err").getElementsByTagName("li").get(0).asNormalizedText());

assertEquals("Password fields must match", page1.getElementById("form:err").getElementsByTagName("li").get(0).asNormalizedText());

Draft Change:

 assertTrue("Validation message not found!", page1.getElementById("<ERROR ID>").getElementsByTagName("li").get(0).asNormalizedText().contains("Password fields must match");

Thanks!

@volosied volosied force-pushed the faces23-validateWholeBean-fix branch from d341c9f to 6b4b853 Compare January 11, 2023 18:47
@arjantijms arjantijms merged commit 3d1817b into jakartaee:master Jan 17, 2023
@pnicolucci pnicolucci added this to the 4.0.1-TCK milestone Feb 26, 2023
@arjantijms arjantijms modified the milestones: 4.0.1-TCK, 4.0.2-TCK Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants