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
Auth Drawer: Use redux store to load settings #85110
Conversation
} catch (error) {} | ||
}, | ||
}) | ||
.then(() => loadSettings(false)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend using async/await approach, it produces much clearer code.
try {
await saveSettings()
await loadSettings(false)
notifyApp.success('Settings saved')
} catch (error) {
notifyApp.error('Failed to save settings')
}
You also could put loadSettings()
call into saveSettings()
action if you always call it after (I think you in most of cases need to re-fetch settings after saving it to display updated values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer promises over async calls, but this is just me. I have changed the code as per your suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided not to include loadSettings
inside saveSettings
because I'm aware they're being used by the old LDAP and SAML UI. I rather keep this clean for now.
Do you think is necessary?
Hello @linoman!
Please, if the current pull request addresses a bug fix, label it with the |
* use actions instead of importing the backend service * Replace the test render for redux-rtl (cherry picked from commit 4e5bff0)
What is this feature?
Improves code by using existing actions to fetch and retrieve settings information.
Why do we need this feature?
The original implementation used the backend service to update the backend settings. While this works, it could be improved.
Who is this feature for?
IAM
Which issue(s) does this PR fix?:
Fixes #
Special notes for your reviewer:
Please check that: