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

Connect sagas to the actual SUSE Manager config UI #2356

Merged
merged 3 commits into from
Feb 23, 2024

Conversation

dottorblaster
Copy link
Contributor

@dottorblaster dottorblaster commented Feb 21, 2024

Description

Connecting the actual settings page to the settings API for SUSE Manager integration.

How was this tested?

Jest tests updates, added new ones

@dottorblaster dottorblaster self-assigned this Feb 21, 2024
@dottorblaster dottorblaster force-pushed the connect-sagas-suse-manager-edit branch 4 times, most recently from d1741a9 to 05f8871 Compare February 22, 2024 09:06
@dottorblaster dottorblaster marked this pull request as ready for review February 22, 2024 09:07
Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Looking great!

Besides the two minor comments the main issues I found are:

  • on saving when the certificate is left empty in the ui ca_cert: "" is passed to the api which does not pass validation. If the certificate is not wanted then the ca_cert property should not be in the api request payload.
  • similarly, when patching settings if we want to remove the certificate, we need to provide the api with ca_cert: null instead of ca_cert: "" otherwise the same validation would not pass.

assets/js/state/selectors/softwareUpdatesSettings.js Outdated Show resolved Hide resolved
assets/js/state/selectors/softwareUpdatesSettings.js Outdated Show resolved Hide resolved
@dottorblaster
Copy link
Contributor Author

@nelsonkopliku thanks for catching that behavior, I applied the fixes 👍

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Almost there.

Saving is working correctly, ca_cert is not sent if the user did not provide any 👍

On update, however, we need an explicit ca_cert: null if we want to remove the certificate, currently the field is not sent at all, meaning it remains untouched.

Besides this the Remove action seems to not be retained in the modal when changing creds like cert or password.

@dottorblaster dottorblaster force-pushed the connect-sagas-suse-manager-edit branch 2 times, most recently from 25f08cb to e2f0491 Compare February 22, 2024 16:08
@dottorblaster
Copy link
Contributor Author

@nelsonkopliku thanks! It should be okay now.

Besides this the Remove action seems to not be retained in the modal when changing creds like cert or password.

I didn't quite understand this, but I guess you mean that closing and reopening the modal should display the Remove buttons again. I think I did something about that.

Copy link
Member

@nelsonkopliku nelsonkopliku left a comment

Choose a reason for hiding this comment

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

Yes, you got it right! I was referring to that.

Besides the CI, LGTM. Great job!

@dottorblaster
Copy link
Contributor Author

Thank you! 😄

@dottorblaster dottorblaster merged commit 6255d13 into main Feb 23, 2024
24 checks passed
@dottorblaster dottorblaster deleted the connect-sagas-suse-manager-edit branch February 23, 2024 09:16
EMaksy pushed a commit that referenced this pull request Feb 26, 2024
* Connect sagas to the actual SUSE Manager config UI

* Apply fixes from review

* Fix linting error about else after return
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.

None yet

3 participants