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

Introduce UI changes for ForensicArtifacts indicator type #146

Merged
merged 8 commits into from
Feb 8, 2024
Merged

Conversation

tomchop
Copy link
Contributor

@tomchop tomchop commented Feb 6, 2024

No description provided.

@tomchop tomchop marked this pull request as ready for review February 8, 2024 21:42
@tomchop tomchop requested a review from udgover February 8, 2024 21:42
Copy link
Collaborator

@udgover udgover left a comment

Choose a reason for hiding this comment

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

Two questions, otherwise LGTM.

Comment on lines +87 to +89
.filter(detail => detail["loc"][2].toLowerCase() === this.typeDefinition.type)
.map(detail => {
return { field: detail.loc[1], message: detail.msg };
return { field: detail["loc"][3], message: detail.msg };
Copy link
Collaborator

Choose a reason for hiding this comment

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

just for my own understanding. Why are you now using 2 for type checking and returning 3 for field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FastAPI returns a weird structure that I had to guess how to parse. the [2] item is which Class the error is comming from (Regexp, ForensicArtifact) and the [3] item is which field in the Class is giving that error.

{ field: "name", type: "text", label: "Name", displayList: true, editable: true },
{ field: "pattern", type: "code", label: "Pattern", displayList: false, editable: true },
{ field: "relevant_tags", type: "list", label: "Relevant tags", displayList: true, editable: true },
{ field: "location", type: "text", label: "Location", displayList: true, editable: true },
{ field: "location", type: "text", label: "Location", displayList: false, editable: true },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it's not worth displaying location?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the list, it takes a lot of space and adds little value. I guess it probably depends what your dataset looks like. Ideally, we could turn this into a toggle so users can pick what columns to display. 💡

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#147 for good measure

Copy link
Collaborator

@udgover udgover left a comment

Choose a reason for hiding this comment

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

LGTM!

@tomchop tomchop merged commit 2260de6 into main Feb 8, 2024
1 check passed
@tomchop tomchop deleted the artifacts branch February 8, 2024 21:58
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.

2 participants