-
Notifications
You must be signed in to change notification settings - Fork 30
Added Dashboard UI enhancements, user can control whether the delete dialog is present or not #107
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
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c266a5fd1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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.
@romil-bijarnia looking for some feedback on the comments provided here. It would be beneficial to understand the ENABLE_POWERSHELL_CONTROLS thought process before approving this one.
I personally dislike the ability to delete without a confirmation and think it should always require a second click, especially where data required for audits is concerned. Is this primarily a development-based function that is aimed at developers, rather than end users?
My final thought - once again we have a pull request that covers 3 distinct areas (backend, engine and frontend). We really do need to stop making changes across distinct areas like this, and separate them into their own pull requests. There are several reasons for this, most notably the fact that this will result in significant issues down the track when the contributor base is much larger and there are distinct teams for these areas.
backend-api/alembic/versions/j2k3l4m5n678_add_user_settings_table.py
Outdated
Show resolved
Hide resolved
|
I quite like the dashboard revamp and think this feels more intuitive. However, there are a couple of inconsistencies.
|
48b894d to
4c266a5
Compare
…shown/not while deleting scans, added compliance trend as an evaluation metric in the dashboard
2dc026e to
3d2830b
Compare
David, regarding the deletable scans, it is dev only. I can for sure be changed later on as needed. |
engine/policies/cis/microsoft-365-foundations/v6.0.0/2.1.7_AntiPhishing_Policy_is_created.rego
Outdated
Show resolved
Hide resolved
engine/policies/cis/microsoft-365-foundations/v6.0.0/metadata.json
Outdated
Show resolved
Hide resolved
| policy_group := "Logging and Monitoring" | ||
|
|
||
| <<<<<<< HEAD | ||
| ======= |
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 noticed some leftover merge-conflict markers (e.g., ======= and >>>>>>>) that slipped in from a previous merge/rebase. I found them and removed them immediately so the file is clean again.
Below are the list of modifications I have made as a part of this pull request:
the dialog being used for delete is a browser native confirm at this stage(like the other dialogs), we should build a custom modal for dialogs later.
I have also added a compliance trend as part of the visualization metric, let me know what everyone's thoughts are.