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

Auth Drawer: Use redux store to load settings #85110

Merged
merged 6 commits into from Mar 26, 2024

Conversation

linoman
Copy link
Contributor

@linoman linoman commented Mar 25, 2024

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:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@linoman linoman self-assigned this Mar 26, 2024
@linoman linoman marked this pull request as ready for review March 26, 2024 09:17
@linoman linoman requested a review from a team as a code owner March 26, 2024 09:17
@linoman linoman added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Mar 26, 2024
} catch (error) {}
},
})
.then(() => loadSettings(false))
Copy link
Contributor

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).

Copy link
Contributor Author

@linoman linoman Mar 26, 2024

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.

Copy link
Contributor Author

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?

@linoman linoman requested a review from Clarity-89 March 26, 2024 12:44
@linoman linoman modified the milestones: 11.0.x, 11.1.x Mar 26, 2024
@linoman linoman added the backport v11.0.x Mark PR for automatic backport to v11.0.x label Mar 26, 2024
Copy link
Contributor

Hello @linoman!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@linoman linoman merged commit 4e5bff0 into main Mar 26, 2024
21 of 22 checks passed
@linoman linoman deleted the linoman/auth_drawer-use_redux branch March 26, 2024 13:56
grafana-delivery-bot bot pushed a commit that referenced this pull request Mar 26, 2024
* use actions instead of importing the backend service

* Replace the test render for redux-rtl

(cherry picked from commit 4e5bff0)
linoman added a commit that referenced this pull request Mar 26, 2024
Auth Drawer: Use redux store to load settings (#85110)

* use actions instead of importing the backend service

* Replace the test render for redux-rtl

(cherry picked from commit 4e5bff0)

Co-authored-by: linoman <2051016+linoman@users.noreply.github.com>
@ashharrison90 ashharrison90 modified the milestones: 11.1.x, 11.0.0-preview Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend backport v11.0.x Mark PR for automatic backport to v11.0.x missing-labels no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants