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

[backend/frontend] Be able to add images in overview of some entity types (#issue/2540) #3998

Merged
merged 60 commits into from
Aug 26, 2023

Conversation

SarahBocognano
Copy link
Member

This issue is the following of Threat Actor Individual Image Carousel. Now we can manage the pictures uploaded in the Data Section, select their order in the carousel, add a description and choose if we want it to be in the carousel or not.

Proposed changes

  • Add a picture management section to be able to order, add description and select carousel selection or not

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
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

@Kedae Kedae added the filigran team use to identify PR from the Filigran team label Aug 8, 2023
@Kedae Kedae linked an issue Aug 8, 2023 that may be closed by this pull request
8 tasks
@yassine-ouaamou
Copy link
Member

Here are some feedbacks we can talk about tomorrow:

  • Last History is unreadable
    image

  • No limit in Image size (maybe store thumbnails or smaller version and show it because loading a 15Mo picture takes time)

  • Alignment of cells in picture management
    image

  • "In Carousel" checkbox becomes false after editing Description or Order

@Archidoit
Copy link
Member

Archidoit commented Aug 16, 2023

When we click in the overview of a threat actor individual, the image displayed is the first one, but the points at the bottom don't correspond (the second point is highlighted in the capture below whereas the image is the first one):
image

@SarahBocognano Yes I know, it's how the carousel library choosed to display the selected image. It's weird to not highlight it, but they choose to shadow the selected one instead

@Archidoit
Copy link
Member

Archidoit commented Aug 16, 2023

Missing translations: 'Pictures management', 'in carousel' and 'update a picture'.
image

image

@SarahBocognano SarahBocognano force-pushed the issue/2540 branch 2 times, most recently from 2db78f5 to c202eea Compare August 21, 2023 13:14
@@ -4230,6 +4222,217 @@ input IndividualAddInput {
file: Upload
}

################ Organizations
enum OrganizationsOrdering {
Copy link
Member

Choose a reason for hiding this comment

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

all of this organizations part is in organization.graphql, maybe a merge issue ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes It's a merge issue

Copy link
Member

Choose a reason for hiding this comment

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

it's still a duplicate no? you don't need to remove it ?

@SarahBocognano SarahBocognano marked this pull request as draft August 23, 2023 07:21
@SarahBocognano SarahBocognano force-pushed the issue/2540 branch 2 times, most recently from 57f6c3e to 2cd8517 Compare August 25, 2023 07:06
@SarahBocognano SarahBocognano marked this pull request as ready for review August 25, 2023 07:51
@Archidoit
Copy link
Member

Archidoit commented Aug 25, 2023

What about making the boxes clickable ? (avoid to open the edition form)
image

@SarahBocognano As discussed with Julien, we choose to make the boxes clickable inline disabled to keep the consistency of the platform. We plan to make editable "lines" in the future, so that way we can avoid forms sometimes, but for now it was asked to be disabled

@Archidoit
Copy link
Member

Edit an image that has an order from Picture Management
Remove the 'order'
Update
An error occurs
image

That's strange because you can have images without order, but when an image has an order, you can't remove it.

@Archidoit
Copy link
Member

Archidoit commented Aug 25, 2023

When no order, we don't see the tooltip when the mouse is over it
image

@Archidoit
Copy link
Member

Archidoit commented Aug 25, 2023

What about enable the sort of the images by order in the carousel? (it's only a suggestion)
image

@Archidoit
Copy link
Member

Error when going to an entity 'Data' tab.
Steps to reproduce:

  • Go to a malware overview (or an other entity).
  • Go to the 'Data' tab
  • An error occurs

image

image

@@ -145,6 +160,7 @@ GenericAttackCardProps
addBookmark(cardData.id, entityType);
}
};
const image = cardData.images?.edges ?? [];
Copy link
Member

Choose a reason for hiding this comment

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

images in plural since it's an array.
Also, maybe it could be useful to have directly the avatarImage here, like this :

const images = cardData.images?.edges ?? [];
const avatarImage = (images.length > 0 && images[0]) ? images[0].node : null;

And then you could directly use avatarImage, and only test if it's defined or not. What do you think ?

/>
) : (
<ItemIcon type={entityType} size="large" />
)
}
title={cardData.name}
title={renderThreatActorIndividual(cardData)}
Copy link
Member

Choose a reason for hiding this comment

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

This component is generic and used for Malware, Campaign, Intrusion Sets, ... not only Threat Actors individuals. How does it work for them ?

@@ -162,7 +165,7 @@ class StixDomainObjectBookmarkComponent extends Component {
/>
</Avatar>
)}
title={node.name}
title={renderThreatActorIndividual(node)}
Copy link
Member

Choose a reason for hiding this comment

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

Same question here as for GenericAttackCard : is this method generic or specific for Threat Actors Individuals ? StixDomainObjectBookmark is a shared component.

@@ -86,6 +89,34 @@ export const computeDuplicates = (fields, data) => R.groupWith(R.allPass(R.map(R

export const capitalizeFirstLetter = (str) => str.charAt(0).toUpperCase() + str.slice(1);

export const renderThreatActorIndividual = (threatActorIndividual) => {
if ((threatActorIndividual.locatedAtCountries?.edges ?? []).length > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need more than one country ? If we need only one, maybe it could be handled by ThreatActorIndividual resolver to return the country ?

@SamuelHassine SamuelHassine merged commit a785a65 into master Aug 26, 2023
4 of 6 checks passed
@SamuelHassine SamuelHassine deleted the issue/2540 branch August 26, 2023 08:33
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.

Be able to add images in overview of some entity types
6 participants