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

Activity log settings UI state #2727

Merged
merged 25 commits into from
Jul 3, 2024
Merged

Activity log settings UI state #2727

merged 25 commits into from
Jul 3, 2024

Conversation

balanza
Copy link
Member

@balanza balanza commented Jun 28, 2024

Description

Mount the Audit Logs section in the Settings Page.
The section at startup fetches the settings data from the backend and render them in the UI. To edit, a modal is open with editable form fields.
The database is prefilled with default settings, so there is no empty data scenario.

Changes

  • rename field retentionTime to retention_time from Components for Activity Logs settings #2706 to adhere to API naming
  • define reducer, selectors and sagas for handling the fetch and the update of the audit logs settings
  • map the api operations
  • mount the redux state
  • mount the UI components in the SettingsPage page.

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.

Cool stuff! I left a few suggestions we might want to take into account.

Besides those, what do you think about having min={1} in the InputNumber inside TimeSpan?

It could help us in making illegal state unrepresentable and possibly simplifying a bit the frontend by being bit more loose in validation handling from the backend.

Happy to answer any question.

assets/js/pages/SettingsPage/SettingsPage.test.jsx Outdated Show resolved Hide resolved
assets/js/pages/SettingsPage/SettingsPage.test.jsx Outdated Show resolved Hide resolved
assets/js/pages/SettingsPage/SettingsPage.test.jsx Outdated Show resolved Hide resolved
assets/js/state/activityLogsSettings.test.js Outdated Show resolved Hide resolved
assets/js/state/activityLogsSettings.test.js Outdated Show resolved Hide resolved
assets/js/state/selectors/activityLogsSettings.js Outdated Show resolved Hide resolved
assets/js/state/selectors/activityLogsSettings.js Outdated Show resolved Hide resolved
assets/js/state/selectors/activityLogsSettings.test.js Outdated Show resolved Hide resolved
assets/js/state/selectors/activityLogsSettings.js Outdated Show resolved Hide resolved
assets/js/state/selectors/activityLogsSettings.js Outdated Show resolved Hide resolved
yield put(setEditingActivityLogsSettings(false));
yield put(setActivityLogsSettingsErrors([]));
} catch (error) {
const errors = get(error, ['response', 'data', 'errors'], []);
Copy link
Member

Choose a reason for hiding this comment

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

We might be missing the opportunity to give the user a proper feedback on generic errors that might not fall into validation ones or that do not satisfy the expectation of a specific shape in the response body.

I forced the update endpoint to return 500, and I think it might be valuable to provide some feedback like unable to save retention time (or whatever message).

Screencast.from.2024-07-01.10-55-35.mp4

I am afraid a non-feedback might be misleading.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. We can add a label aside the saving buttons, for global errors.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added generic error in 09bea2e.

The UI simply shows the message Something went wrong while saving when an unstructured error appears (see storybook). We might provide a more descriptive message if we can add some intelligence to the error parsing, though..

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved in 52303ca

@nelsonkopliku nelsonkopliku added the env Create an ephimeral environment for the pr branch label Jul 1, 2024
@balanza balanza force-pushed the activity-log-settings-ui-state branch from 918148f to e2f6827 Compare July 2, 2024 07:38
@balanza balanza force-pushed the activity-log-settings-ui-state branch from 8a8c5a1 to c058961 Compare July 2, 2024 11:23
@balanza balanza requested a review from nelsonkopliku July 2, 2024 11:36
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.

Great! Thanks!

@balanza balanza merged commit 5af8fac into main Jul 3, 2024
26 checks passed
@balanza balanza deleted the activity-log-settings-ui-state branch July 3, 2024 13:21
@balanza balanza mentioned this pull request Jul 3, 2024
@nelsonkopliku nelsonkopliku added enhancement New feature or request user story ux labels Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request env Create an ephimeral environment for the pr branch user story ux
Development

Successfully merging this pull request may close these issues.

2 participants