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

[frontend] Add content mapping in content tab (#5651) #7136

Merged
merged 11 commits into from
Jun 13, 2024

Conversation

marieflorescontact
Copy link
Member

@marieflorescontact marieflorescontact commented May 27, 2024

  • remove content mapping from knowledge Tab for container
  • add content mapping in content Tab for Grouping/Report/Cases
  • add root component to switch from content to content mapping in Tab content

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case (coverage and e2e)
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

@marieflorescontact marieflorescontact force-pushed the issue/5651-chunk2 branch 2 times, most recently from a6492b1 to 8a9cf76 Compare May 29, 2024 12:40
@marieflorescontact marieflorescontact changed the title [frontend] Add StixCoreObjectContentHeader and StixCoreObjectContentR… [frontend] Add content mapping in content tab (#5651) May 29, 2024
@marieflorescontact marieflorescontact marked this pull request as ready for review May 29, 2024 14:46
@marieflorescontact marieflorescontact force-pushed the issue/5651-chunk2 branch 2 times, most recently from d4cdccf to f2c84b3 Compare May 30, 2024 05:24
Copy link

codecov bot commented May 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.21%. Comparing base (08932ab) to head (65fa491).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7136      +/-   ##
==========================================
+ Coverage   67.75%   68.21%   +0.45%     
==========================================
  Files         561      561              
  Lines       68824    68891      +67     
  Branches     5871     6460     +589     
==========================================
+ Hits        46631    46992     +361     
+ Misses      22193    21899     -294     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@SouadHadjiat
Copy link
Member

SouadHadjiat commented May 30, 2024

Description seem to be always empty in content mapping view (tested for serveral reports)

Capture d'écran 2024-05-30 153052

image

Even content is empty, when trying to create a report with a content, and selecting "create and map", the content mapping view displays empty content.

@marieflorescontact marieflorescontact force-pushed the issue/5651-chunk2 branch 2 times, most recently from 148739e to 0d2597f Compare May 31, 2024 12:21
@marieflorescontact
Copy link
Member Author

marieflorescontact commented May 31, 2024

Description seem to be always empty in content mapping view (tested for serveral reports)

Fixed

@marieflorescontact
Copy link
Member Author

all comments have been taken into account, ready for another review

@JeremyCloarec JeremyCloarec added the filigran team use to identify PR from the Filigran team label Jun 6, 2024
return location.pathname;
};

export const getPaddingRight = (location: Location, entity_id: string, entity_type_path: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for refactoring this part: just one small question: since we only use location.pathname, I don't see the use to have the whole Location object in parameter, maybe locationPath is enough ? same question for getCurrentTab.

@SouadHadjiat
Copy link
Member

I'm wondering about the icon used in content for content view which is the same as for graph view, but we only display the content (content & files), shouldn't we use another icon?

image

@marieflorescontact
Copy link
Member Author

I'm wondering about the icon used in content for content view which is the same as for graph view, but we only display the content (content & files), shouldn't we use another icon?

image

yes we should definitly use another icon :) @nino-filigran

@nino-filigran
Copy link

Can't we use a toggle? Since the action is more to "enable/disable" a mode @marieflorescontact

@marieflorescontact
Copy link
Member Author

Can't we use a toggle? Since the action is more to "enable/disable" a mode @marieflorescontact

as discussed with @nino-filigran we will use :
image

@marieflorescontact
Copy link
Member Author

all comments have been taken into account, ready for another review

@SouadHadjiat
Copy link
Member

SouadHadjiat commented Jun 7, 2024

Content mapping view works all right ✅

However there seem to be a padding issue here :

image

image

@SouadHadjiat
Copy link
Member

There is an issue now in reports -> knowledge, the graph should take the whole width :

image

@marieflorescontact marieflorescontact self-assigned this Jun 11, 2024
@marieflorescontact
Copy link
Member Author

There is an issue now in reports -> knowledge, the graph should take the whole width :

fixed

@marieflorescontact
Copy link
Member Author

all comments have been taken into account, ready for another review

@SouadHadjiat
Copy link
Member

image

@SouadHadjiat
Copy link
Member

"knowledge" tab is not selected here in threat actor group

image

@SouadHadjiat
Copy link
Member

Feedback has a content mapping button :
Capture d'écran 2024-06-12 102903

And infrastructure doesn't have "knowledge" current tab selected:
Capture d'écran 2024-06-12 103134

@marieflorescontact
Copy link
Member Author

all comments have been taken into account, ready for another review

@SouadHadjiat
Copy link
Member

SouadHadjiat commented Jun 12, 2024

just found another one with wrong padding:

image

But it's actually already like this in production, so not a regression of this PR

@marieflorescontact marieflorescontact merged commit e364f82 into master Jun 13, 2024
5 checks passed
@marieflorescontact marieflorescontact deleted the issue/5651-chunk2 branch June 13, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants