You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "mvolf (via GitHub)" <gi...@apache.org> on 2024/01/26 15:10:26 UTC
[PR] Import file validation [camel-karavan]
mvolf opened a new pull request, #1085:
URL: https://github.com/apache/camel-karavan/pull/1085
@mgubaidullin Here's an implementation for validating file existence on project file import/upload. I've also propagated the same validation on file creation, so it's uniform and both uses the generic validator previously implemented.
There also a small fix in getPersonIdent method to use default e-mail address if it's not defined in UserInfo.
Also fixed a couple of quarkus quinoa properties, because quinoa was not working after the version upgrade.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mgubaidullin (via GitHub)" <gi...@apache.org>.
mgubaidullin commented on PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#issuecomment-1914833115
@mvolf your validation framework is great. And it might be good part of any application with a lot of forms and validations. In the Karavan we have only 2 forms (new project and new files) to validate on server-side (exists/not-exists) and both forms have 4 fields total.
And I do not see any other forms that we might need to validate server-side in the future.
However let's keep the validation framework for now to move forward and reconsider later when we have more observations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mgubaidullin (via GitHub)" <gi...@apache.org>.
mgubaidullin commented on code in PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#discussion_r1468114445
##########
karavan-web/karavan-app/src/main/webui/src/topology/TopologyToolbar.tsx:
##########
@@ -34,7 +34,7 @@ export function TopologyToolbar (props: Props) {
<ToolbarContent>
<ToolbarItem align={{default:"alignRight"}}>
<Tooltip content={"Add Integration Route"} position={"bottom"}>
- <Button className="dev-action-button" size="sm"
Review Comment:
Please keep classname
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mgubaidullin (via GitHub)" <gi...@apache.org>.
mgubaidullin commented on code in PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#discussion_r1468114602
##########
karavan-web/karavan-app/src/main/webui/src/topology/TopologyToolbar.tsx:
##########
@@ -45,7 +45,7 @@ export function TopologyToolbar (props: Props) {
</ToolbarItem>
<ToolbarItem align={{default:"alignRight"}}>
<Tooltip content={"Add REST API"} position={"bottom"}>
- <Button className="dev-action-button" size="sm"
Review Comment:
Please keep classname
##########
karavan-web/karavan-app/src/main/webui/src/topology/TopologyToolbar.tsx:
##########
@@ -56,7 +56,7 @@ export function TopologyToolbar (props: Props) {
</ToolbarItem>
<ToolbarItem align={{default:"alignRight"}}>
<Tooltip content={"Add Bean"} position={"bottom"}>
- <Button className="dev-action-button" size="sm"
Review Comment:
Please keep classname
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mgubaidullin (via GitHub)" <gi...@apache.org>.
mgubaidullin commented on PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#issuecomment-1912596629
@mvolf thank for the contribution.
This Validator on backend and and yup on frontend with dosens lines of code just to simply check if file is already exists.
Look like an overkill. Should it be that complex?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mvolf (via GitHub)" <gi...@apache.org>.
mvolf commented on PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#issuecomment-1913511445
> @mvolf thank for the contribution. This Validator on backend and and yup on frontend with dosens lines of code just to simply check if file is already exists. Look like an overkill. Should it be that complex?
Thanks for the comments @mgubaidullin.
I personally don't see this as complex or as an overkill. Validation is one of the most important parts of the system, since it ensures data validity/consistency and makes the system less prone to serious bugs.
In this example I don't look at this as "simply check if file already exists", but as potential consequences if this kind of validation is not implemented properly through out the whole system (as we've seen this results in overwriting of users existing file).
This kind of approach also follows several best approaches:
* back-end should always do the validation (preferably service method). In this way validation is enforced across the system where ever this method is called. Validation is also enforced if API is called manually, which bypasses any front-end validation.
* validation error messages are a part of the back-end validator and are shown on the front-end. This makes translation of these messages fairly easy to implement if needed.
* additional front-end validation (in our case using react-hook-form and yup) is added for presenting users with certain validation messages while typing, to speed up the flow and unburden back-end by avoiding unnecessary API calls.
* implementation of the validation is uniform and follows the same principles, where implementing validation differently for every page depending on the needs makes the code less clear and harder to maintain
These are some of the main points, but there are several more benefits.
If you look more closely to this implementation and compare it to the existing you'll notice that there's not that much code more and in my opinion the benefits far outweigh a bit more lines of the code.
I hope that I managed to present you my point of view :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mgubaidullin (via GitHub)" <gi...@apache.org>.
mgubaidullin commented on PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#issuecomment-1912758224
It would be great if we keep `axios` stuff in one place KaravanApi instead of spreading over the app.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mgubaidullin (via GitHub)" <gi...@apache.org>.
mgubaidullin merged PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mvolf (via GitHub)" <gi...@apache.org>.
mvolf commented on PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#issuecomment-1913512564
> It would be great if we keep `axios` stuff in one place KaravanApi instead of spreading over the app.
@mgubaidullin I'm not sure what you're aiming at with this comment :) All the axios stuff is in the KaravanApi. If you're taking about the import of the AxiosError in the UploadFileModal and CreateFileModal, that's only because Typescripts needs explicit type for the method parameter.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Import file validation [camel-karavan]
Posted by "mvolf (via GitHub)" <gi...@apache.org>.
mvolf commented on code in PR #1085:
URL: https://github.com/apache/camel-karavan/pull/1085#discussion_r1468785276
##########
karavan-web/karavan-app/src/main/webui/src/topology/TopologyToolbar.tsx:
##########
@@ -34,7 +34,7 @@ export function TopologyToolbar (props: Props) {
<ToolbarContent>
<ToolbarItem align={{default:"alignRight"}}>
<Tooltip content={"Add Integration Route"} position={"bottom"}>
- <Button className="dev-action-button" size="sm"
Review Comment:
I'm not sure why this changed. I didn't even open this file, let alone changed anything in it. Is it possible that the build I ran generated these changes? In any case I'll revert them manually.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org