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

Validation Review UI #121

Merged
merged 23 commits into from
Oct 22, 2024
Merged

Validation Review UI #121

merged 23 commits into from
Oct 22, 2024

Conversation

Tyouxik
Copy link
Collaborator

@Tyouxik Tyouxik commented Oct 7, 2024

New features

  • Threat model validation is visible in the bottom panel
image
  • Validation step happens before the threat model can be sent for review
image

app/package.json Outdated Show resolved Hide resolved
@Tethik Tethik changed the title Review UI 1 Validation Review UI Oct 7, 2024
@Tethik
Copy link
Contributor

Tethik commented Oct 8, 2024

I think the MUI upgrade broke the Team system lists on the Home page:
image

@Tethik
Copy link
Contributor

Tethik commented Oct 8, 2024

image

@Tethik
Copy link
Contributor

Tethik commented Oct 8, 2024

  • Remember to clean up your console.log before merge - they should not be used in release.

Sorry, something went wrong.

@Tethik
Copy link
Contributor

Tethik commented Oct 8, 2024

More general feedback:

Quality Check Popup

  • As a user, if I go to request a review and have never seen the Quality Check / Validation before, I don't know how I'm supposed to open the validation panel. There is nothing informing that it exists until you press "Request Review" and even so you don't know that you can open the panel. Users may end up opening that Quality Check popup over and over instead of checking the bottom panel.
  • The popup could be taller/longer, currently the scroll kicks in a bit too early I think.
  • Percentage shown is not explained what it means
    • It would be cool if we calculated this serverside and had it via an API, that way we could use it later for e.g. Target Specifications / Violations.
    • Also probably should calculate this score based on weights. Some rules are likely less important than others.

Bottom Tab

  • Model tab is not clear to a user what it's meant for
  • I don't feel like the button that opens the validation panel belongs in the control panel, but I don't have any better suggestion so I think we can leave it there for now.
  • Maybe in the future we should consider removing the tabs to simplify into a single "Validation" tab.

Indicator

  • Maybe we should reuse the Indicators already on the components to signal validation?

Rules / Validation backend

  • With the current StaticValidator ruleset, how would I add a rule that checks if a component has at least one threat/control? I don't see any dal reference passed which would allow to make those lookups in the database. Maybe it should allow for being more dynamic (I know, static is in the name).
  • The validateModel route itself does not follow conventions from other api routes (I can probably refactor this quickly).

KPIs / Metrics

  • One potential way to track how useful this feature is could be to start measuring the review statuses - does this feature result in less meetings?
  • Another thing to measure over time is the average quality score.

addComponent({ name, type, x: pos.x, y: pos.y });
}}
/>
{!isFramed && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Toolbar should still be shown when Framed

const successRatio = (
passedResults.length / validationResults.length
).toFixed(2);
function createQualityMessage(successRatio) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function can be moved outside the component. As in same file, just before the definition of the Component Function.

if (!isModelValidation(rule)) {
continue;
}
const testResult = rule.test(model);
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it runs all rules synchronously, consider using promises?

return <></>;
}
return (
<Box sx={{ gridArea: "bottom", backgroundColor: "rgb(40,40,40)" }}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Box sx={{ gridArea: "bottom", backgroundColor: "rgb(40,40,40)" }}>
<Box sx={{ gridArea: "bottom", backgroundColor: "rgb(30,30,30)" }}>

This makes it darker and not look blobby with the other panels.

import { modalActions } from "../../../redux/modalSlice";
import { MODALS } from "../../elements/modal/ModalManager";

export function ValidateBeforeReview() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming this to QualityCheck or similar instead, makes it easier to find.

@Tyouxik Tyouxik requested a review from Tethik October 22, 2024 12:14
return res.sendStatus(400);
if (dal.validationEngine.rules.length === 0) {
return res
.status(404)
Copy link
Contributor

Choose a reason for hiding this comment

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

404 here doesn't make sense. It should just return 200 with an empty list. Or are you handling this on the frontend?

Copy link
Contributor

Choose a reason for hiding this comment

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

400 errors usually denote client errors, which this is not.


if (validationResults.length === 0) {
return res
.status(404)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, odd use of 404

@Tyouxik Tyouxik merged commit a19ab9c into main Oct 22, 2024
1 check passed
@Tyouxik Tyouxik deleted the review-ui-1 branch November 14, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants