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

[#55382] add connection validation section #15894

Merged

Conversation

Kharonus
Copy link
Member

#55382

WAT?

  • added connection validation section to sidebar
    • allows manual request of a health check for this storage
    • checks for tenant id, drive id, oauth credentials, AMPF content
  • added new controller for connection validation
  • show connection validation only for one drive storages

@Kharonus Kharonus self-assigned this Jun 19, 2024
@Kharonus Kharonus force-pushed the implementation/55382-add-connection-validation-one-drive-storages branch from 370e37e to c97535a Compare June 20, 2024 12:59
@Kharonus Kharonus requested a review from a team June 21, 2024 07:52
Copy link
Member

@akabiru akabiru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one @Kharonus 🏆 I have a couple of 🟢 & 🟠 but nothing blocking. Shout if easier to chat/pair 🙂

Gemfile Show resolved Hide resolved
Comment on lines 43 to 50
maybe_is_not_configured
.or { tenant_id_wrong }
.or { client_id_wrong }
.or { client_secret_wrong }
.or { drive_id_wrong }
.or { request_failed_with_unknown_error }
.or { drive_with_unexpected_content }
.value_or(ConnectionValidation.new(type: :healthy, timestamp: Time.current, description: nil))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 The monads are a really nice touch, but I wonder if we can follow a positive naming scheme- the negation based naming can sometimes be confusing (at least to me)

Suggested change
maybe_is_not_configured
.or { tenant_id_wrong }
.or { client_id_wrong }
.or { client_secret_wrong }
.or { drive_id_wrong }
.or { request_failed_with_unknown_error }
.or { drive_with_unexpected_content }
.value_or(ConnectionValidation.new(type: :healthy, timestamp: Time.current, description: nil))
maybe_storage_configured
.or { validate_tenant_id }
.or { validate_client_id }
.or { validate_client_secret }
.or { validate_drive_id }
.or { request_failed_with_unknown_error }
.or { drive_with_unexpected_content }
.value_or(ConnectionValidation.new(type: :healthy, timestamp: Time.current, description: nil))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, well, that is interesting. Especially due to the or concatenation I naturally read negative namings. It seems more natural then the positive way.

See this imaginary monologue.

"So, I have this storage.... I suspect it to be invalid."
"Maybe the storage is not fully configured?"
"Well, apparently it is... so, is then tenant id wrong?"
"Hmm, not... what about the client id?"
"Or the client secret?"
"Anything of that? No? Then apparently you are valid."

Get what I am pointing at? :D validations are "positive" when they are "negative", hence finding a problem means, your validation was justified.

Copy link
Member

@akabiru akabiru Jun 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get what I am pointing at? :D validations are "positive" when they are "negative", hence finding a problem means, your validation was justified.

I see wym for sure, and perhaps ActiveModel::Validations are a great conventional example; they only push to errors in the even "negative" event that there's a validation failure, otherwise they just return ?true.

Hence in your imaginany monologue 😄 mine would read like so:

"So I have this storage... I need to validate that everything is configured properly"
"Firstly, validated it is fully configured"
"Then, validate the tenant id is correct"
"Then, validate the client id is correct"
"Then, validate the client secret is correct"
"When there are invalid cases, collect them and display a unified response (similar to active_model_errors) to the user"
"When there are no errors, return- mark the storage connection as valid"


I reckon what's different with my monologue is that I presume we're collecting all errors- and displaying them once to the user, but AFAICT- we fail fast on the first error result? In which case that might be inconvenient if the user has to click the button multiple times because they are served one error at a time.

In which case, I wonder if just mixin in ActiveModel::Validations and writing these as validate statements would be more fitting albeit less "Monad-ish" 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but AFAICT- we fail fast on the first error result?

Here's the trick, except for the missing/empty values, it is impossible to validate if the previous validation has failed. If the tenant id is wrong, we can't validate the Client ID/Secret, if the Client ID/Secret is wrong we can't validate the Drive ID.

In which case, I wonder if just mixin in ActiveModel::Validations and writing these as validate statements would be more fitting albeit less "Monad-ish" 😄

There are always more than one way to do things. A dry/validation contract, ActiveModel::Validations or a chain of assertions all could do the same thing.

  1. ActiveModel would be tied directly to an object (with certain assumptions).
  2. Dry::Validation::Contract would create an object just for validation of a hash/json that could be passed down after validation.
  3. A chain of assertions (in this case using the Maybe/Option monad) makes explicit the validation process.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the trick, except for the missing/empty values, it is impossible to validate if the previous validation has failed. If the tenant id is wrong, we can't validate the Client ID/Secret, if the Client ID/Secret is wrong we can't validate the Drive ID.

Yes, and here we should design it, as the domain user story is telling us. And here, it was set, we do not show all errors. We do show the first error, that can happen. Hence, the implementation follows that story, and fails with the first error, not checking for the other validation rules.

Kharonus added 9 commits June 25, 2024 14:44
- https://community.openproject.org/work_packages/55382
- added new controller for connection validation
- added new component to sidebar
- enable sidebar for all one drive storages to show the connection
  validation
- have one health status section, containing both AMPF and connection
  validation
- use helper methods from OpTurbo::Streamable
- add error codes to validation result
@Kharonus Kharonus force-pushed the implementation/55382-add-connection-validation-one-drive-storages branch from 087beb9 to aa732f7 Compare June 25, 2024 13:42
@Kharonus Kharonus requested review from akabiru and mereghost June 26, 2024 08:10
Copy link
Member

@akabiru akabiru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: Cheers Eric!

@Kharonus Kharonus merged commit 697c7a4 into dev Jun 26, 2024
9 checks passed
@Kharonus Kharonus deleted the implementation/55382-add-connection-validation-one-drive-storages branch June 26, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants