eclipse-ee4j / starter

Eclipse Starter for Jakarta EE
Eclipse Public License 2.0
50 stars 38 forks source link

Enforce Validation in UI #190

Closed geziefer closed 1 year ago

geziefer commented 1 year ago

If I select core profile and Jakarta EE version <10 directly in archetype: mvn archetype:generate -DarchetypeGroupId=org.eclipse.starter -DarchetypeArtifactId=jakarta-starter -DarchetypeVersion=2.0-SNAPSHOT -DgroupId=com.example -DartifactId=demo -Dprofile=core -DjakartaVersion=9 -Dversion=1.0.0-SNAPSHOT -DinteractiveMode=false

it results in an error message (which is correct): Failed, the Core Profile is only supported for Jakarta EE 10

But if I do the same from the UI, it results in exception page:

Stack Trace:
javax.servlet.ServletException: java.lang.RuntimeException: Failed to invoke Maven Archetype.
    at javax.faces.api@3.1.0.SP01//javax.faces.webapp.FacesServlet.executeLifecyle(FacesServlet.java:725)
    at javax.faces.api@3.1.0.SP01//javax.faces.webapp.FacesServlet.service(FacesServlet.java:451)
    at io.undertow.servlet@2.2.19.Final//io.undertow.servlet.handlers.ServletHandler.handleRequest(ServletHandler.java:74)
    at io.undertow.servlet@2.2.19.Final//io.undertow.servlet.handlers.FilterHandler$FilterChainImpl.doFilter(FilterHandler.java:129)
...

Same happens if I select Java 8 for Jakarta EE 10, which won't work since Java 11 is minimum requirement.

Also, if I select docker support, I have to select a runtime, otherwise the generation would work, but without docker support.

And, some runtimes do not support the selected version or profile.

So we need to respect the existing dependencies between the selectable options and only enable the possible options for a given Jakarta EE version.

Besides, there should be an error message instead of the exception page.

m-reza-rahman commented 1 year ago

Hi @jeyvison, where are we with merging my latest PR? It adds validation in the UI.

jeyvison commented 1 year ago

Hi @m-reza-rahman , nowhere ^^ I was wating to see if bazlur had answers to your replies. If you feel your replies were eneough we can resolve them and merge it

jeyvison commented 1 year ago

I already have checked and it loogs good to me for a V1.

m-reza-rahman commented 1 year ago

Yes, let’s merge please. I don’t think resolving comments are needed for that.

m-reza-rahman commented 1 year ago

Hi @geziefer, please re-test after https://github.com/eclipse-ee4j/starter/pull/187 is merged. These should all be addressed. If not, let me know which case is not covered yet.

m-reza-rahman commented 1 year ago

Hi @geziefer, the PR is merged. Kindly re-test and share observations?

geziefer commented 1 year ago

Yes will retest it.

nikitakoselev commented 1 year ago

Great, I'll test it too :)

geziefer commented 1 year ago

It seems to me, that all validation problems I saw before, are covered now by UI.

Yet, I think a user without the knowledge of the dependencies among the selection will be at least irritated, if not thinking, the UI is not working right. This is because now, the UI allows for only those selections which are possible among the currently selected. This is not wrong, but from a user perspective a bit strange, since he does want to select a specific option, let's say Docker support, but is not able to do so, since no runtime was selected before. And he does not receive any feedback, why he can't select Docker support at the moment.

So I would suggest to first let the user select whatever he wants, but then show a hint, that a genereation is currently not possible and why and what should be selected. This might be a bit more to implement, but much clearer for the average user.

m-reza-rahman commented 1 year ago

Let’s see what others think. After the fact validation is actually easier to implement. Another possible option to explore is adding a tooltip that shows a reason why something is disabled with the current selection.

m-reza-rahman commented 1 year ago

For now I will go ahead and add a description at the top as to why options are disabled and what to do in order to enable them.

m-reza-rahman commented 1 year ago

I have added an explanation section at the top. Perhaps it's sufficient for now or even overall? The benefit of real-time validation is that the response loop is a lot faster. Anyway, would love to get feedback on a viable approach at least for 2.0.

image

geziefer commented 1 year ago

Yes, real-time validation is fine, but the user should receive feedback.

But I think, instead of the big text block at the beginning, the explanation should be below each option.

geziefer commented 1 year ago

Other than that, I think the ticket can be closed.

m-reza-rahman commented 1 year ago

I am not sure what you mean, but I am thinking it’s doable as long as I can understand it. Can you please provide a screen mock-up? Do you mean just an explanation per option group how things are enabled/disabled? That’s quite a bit of text. Is it better as a tooltip? Is it better to just have a “help” page that outlines all that in one place?

Of course, PRs are always most helpful.

geziefer commented 1 year ago

Ok, I try to come up with a suggestion, ideally in code.

nikitakoselev commented 1 year ago

My major challenge is that I need to read the text, in order to understand why specific radio buttons are not enabled. Let me record a small video and ask the community on Twitter.

nikitakoselev commented 1 year ago

I have found two potential bugs:

1 (small one): once the Eclipse Starter page is loaded - take a note of the last row, "Runtime" and hit F5 key.

The row keeps changing the order of the elements on my machine. Looks the collection is not sorted.

image

2 (big one):

2.1 Open new page 2.2 Set "Jakarta EE profile" to "Full platform" 2.3 Set "Jakarta EE version" to "Jakarta EE 8" 2.4 Set "Java SE version" to "Java SE 8" image 2.5 Now you can't reset Jakarta EE profile to "EE 10" even if you select back to SE 17. image image

3 (small one): "Full platform" text. Why is "platform" not capitalized? Everything else has the second word capitalized on that page

image

nikitakoselev commented 1 year ago

Please let me know if I need to raise separate tickets. I was investigating the ticket and made a small recording, which will be available in an hour: https://youtu.be/Y8dCDcTWOkk

m-reza-rahman commented 1 year ago

My major challenge is that I need to read the text, in order to understand why specific radio buttons are not enabled.

Please propose the alternative you prefer. I doubt there is a "perfect" solution due to the many variations in play. We just need to agree on what is workable enough for the release.

1 (small one): once the Eclipse Starter page is loaded

This is a feature, not a bug. We have to randomly order the runtimes to avoid any possible bias for one or the other.

2 (big one)

This would be a bug. Hopefully is easy to reproduce and fix. I'll move ahead ASAP.

3 (small one): "Full platform" text.

This is because "full" isn't really a profile. It's simply referring to the full platform. Not a big deal to capitalize Platform. I'll go ahead and do it.

Please let me know if I need to raise separate tickets.

No need, unless it helps you.

m-reza-rahman commented 1 year ago

Hi @nikitakoselev, #2 and #3 are addressed. Please re-test?

nikitakoselev commented 1 year ago

Hi, I can confirm both #2 and #3 work now.

I have streamed the testing process on LinkedIn and YouTube , I hope that is OK.

m-reza-rahman commented 1 year ago

Hi @geziefer and @nikitakoselev, based on feedback from IBM/@scottkurz, I have changed the defaults of the form and re-ordered the fields slightly. More options are now enabled by default. Is that an improvement?

Do you think it would help to choose a valid random runtime by default and also enable Docker by default?

geziefer commented 1 year ago

Being sick today, but will try to do something the next days. Alex

Von: Reza Rahman @.> Gesendet: Dienstag, 14. März 2023 04:43 An: eclipse-ee4j/starter @.> Cc: Alexander Rühl @.>; Mention @.> Betreff: Re: [eclipse-ee4j/starter] Enforce Validation in UI (Issue #190)

EXTERNAL SENDER: Be cautious with links and attachments!

Hi @gezieferhttps://github.com/geziefer and @nikitakoselevhttps://github.com/nikitakoselev, based on feedback from @.***https://github.com/scottkurz, I have changed the defaults of the form and re-ordered the fields slightly. More options are now enabled by default. Is that an improvement?

Do you think it would help to choose a valid random runtime by default and also enable Docker by default?

— Reply to this email directly, view it on GitHubhttps://github.com/eclipse-ee4j/starter/issues/190#issuecomment-1467305349, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ACGZT2RENBNKIOHGLWVXK3TW37SNPANCNFSM6AAAAAAVUVOY3Q. You are receiving this because you were mentioned.Message ID: @.***>

nikitakoselev commented 1 year ago

Thank looks so much better, thank you ☺ image

m-reza-rahman commented 1 year ago

Great, if possible I would like to start driving towards a 2.0 release soon. If there are no more major bugs or usability concerns, more minor improvements can always be done later.

geziefer commented 1 year ago

I made a PR, please check if you regard this as an improvement. This is how it looks: image

m-reza-rahman commented 1 year ago

Hi @scottkurz, do you believe this is an improvement? Would you like to tweak the “validation notes” further? I know you had specific ideas on what we should say?

geziefer commented 1 year ago

As my initial concern was addressed by reza and my own enhancement merged, I'll close this one.