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] improved code comments (#1536) #6940

Merged
merged 2 commits into from
May 14, 2024
Merged

[backend] improved code comments (#1536) #6940

merged 2 commits into from
May 14, 2024

Conversation

JeremyCloarec
Copy link
Contributor

@JeremyCloarec JeremyCloarec commented May 13, 2024

Proposed changes

  • improved code comment to explain some design choices

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

Copy link

codecov bot commented May 13, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 67.42%. Comparing base (64d087e) to head (8183cb9).
Report is 2 commits behind head on master.

Files Patch % Lines
.../modules/deleteOperation/deleteOperation-domain.ts 0.00% 4 Missing ⚠️
...pencti-platform/opencti-graphql/src/domain/stix.js 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6940   +/-   ##
=======================================
  Coverage   67.42%   67.42%           
=======================================
  Files         561      561           
  Lines       68510    68518    +8     
  Branches     5713     5714    +1     
=======================================
+ Hits        46191    46199    +8     
  Misses      22319    22319           

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

// only hint the first 3 ones
let name = extractRepresentative(targetsFromTrash[0]).main;
if (targetsFromTrash.length > 1) name = `${name},${extractRepresentative(targetsFromTrash[1]).main}`;
if (targetsFromTrash.length > 2) name = `${name},${extractRepresentative(targetsFromTrash[2]).main}`;
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified like this :

let name = targetsFromTrash.slice(0, 3).map((t) => extractRepresentative(t).main).join(',');
if (targetsFromTrash.length > 3) name = `${name}, ... and ${targetsFromTrash.length - 3} more`;

With a suggestion to display the number of other entities with ', ... and X more'

@JeremyCloarec JeremyCloarec marked this pull request as ready for review May 13, 2024 16:01
@@ -39,6 +39,9 @@ export const stixDelete = async (context, user, id) => {
const element = await internalLoadById(context, user, id);
if (element) {
if (isStixObject(element.entity_type) || isStixRelationship(element.entity_type)) {
// To handle delete synchronization events, we force the forceDelete flag to true, because we don't want delete events to create trash entries on synchronized platforms
// THIS IS NOT IDEAL: we ideally would need to add the forceDelete flag to all delete related methods on the API,
Copy link
Member

Choose a reason for hiding this comment

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

shall we open a new issue to "extend gql API/client-python deletion functions with forceDelete option" and keep track of it for the product team ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's a good idea yes, I'll create the issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

created the issue here #6969

const name = extractRepresentative(targetsFromTrash[0]); // only hint the first one
throw FunctionalError(`Cannot restore: a relationship targets a deleted element [${name.main}], restore it before retrying`);
// only hint the first 3 ones
let name = targetsFromTrash.slice(0, 3).map((t) => extractRepresentative(t).main).join(',');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let name = targetsFromTrash.slice(0, 3).map((t) => extractRepresentative(t).main).join(',');
let name = targetsFromTrash.slice(0, 3).map((t) => extractRepresentative(t).main).join(', ');

@JeremyCloarec JeremyCloarec merged commit 0ca75db into master May 14, 2024
5 checks passed
@JeremyCloarec JeremyCloarec deleted the issue/1536 branch May 14, 2024 12:25
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

3 participants