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.

Sign up for GitHub

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

Jump to bottom

[Frontend/Backend] Skip line when the config give a specific char (#4505) #4815

Merged
merged 1 commit into from Nov 13, 2023
Merged

[Frontend/Backend] Skip line when the config give a specific char (#4505) #4815

merged 1 commit into from Nov 13, 2023

Conversation

jpkha
Copy link
Member

@jpkha jpkha commented Oct 31, 2023 •
edited

Proposed changes

  • Can skip csv line like # when parsing the CSV files.

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

@jpkha jpkha force-pushed the issue/4505-2-bis branch 2 times, most recently from e36764e to 5423948 Compare October 31, 2023 06:52
@codecov
Copy link

codecov bot commented Oct 31, 2023 •
edited

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (184383d) 0.00% compared to head (7519bdc) 0.00%.

❗ Current head 7519bdc differs from pull request most recent head f134256. Consider uploading reports for the commit f134256 to get more accurate results

Additional details and impacted files 
@@      Coverage Diff       @@
##   master   #4815   +/-   ##
==============================
==============================

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

@jpkha jpkha added the filigran team use to identify PR from the Filigran team label Oct 31, 2023
@jpkha jpkha force-pushed the issue/4505-2-bis branch 2 times, most recently from 9629bcf to ab08c3c Compare October 31, 2023 09:54
@jpkha jpkha force-pushed the issue/4505-2-bis branch 3 times, most recently from ec0397e to 8229dff Compare November 2, 2023 06:31
@RomuDeuxfois
Copy link
Member

I reviewed the code and it's good for me. I let other developers do the testing

@jpkha jpkha force-pushed the issue/4505-2-bis branch 8 times, most recently from 082b611 to 7519bdc Compare November 7, 2023 08:41
SouadHadjiat
SouadHadjiat reviewed Nov 7, 2023
View reviewed changes
opencti-platform/opencti-front/src/private/components/data/csvMapper/CsvMapperForm.tsx Outdated
}));

const csvMapperValidation = (t: (s: string) => string) => Yup.object().shape({
name: Yup.string().required(t('This field is required')),
has_header: Yup.boolean().required(t('This field is required')),
separator: Yup.string().required(t('This field is required')),
skipLineChar: Yup.string(),
Copy link
Member

Choose a reason for hiding this comment

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

can we validate the char ? I guess it should be only one character or can it be more ? like this maybe :
Yup.string().max(1)

Copy link
Member Author

Choose a reason for hiding this comment

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

@nino-filigran @Jipegien, what do you think ? should we restrict to one character ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I saw with @Jipegien we will restrict to one character. Thanks @SouadHadjiat.

SouadHadjiat
SouadHadjiat reviewed Nov 7, 2023
View reviewed changes
opencti-platform/opencti-graphql/src/parser/csv-bundler.ts
let records = await parsingProcess(content, mapper.separator);
if (mapper.skipLineChar) {
records = records.filter((record) => !record[0].startsWith(mapper.skipLineChar));
}
Copy link
Member

Choose a reason for hiding this comment

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

what if records is not defined ? or record is empty ?
Maybe it could be done when we loop on records below ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not try to send an empty CSV files. It worth to try the case.
In my opinion, I prefer to do it before the record because inside the records, we have an option to skip the line for the header. This will help separate the different use cases and improve readability. If we put it inside, we would need to check and manage if we are on the first line of the files or in a comment line, which would bring more complexity. What do you think?

labo-flg
labo-flg reviewed Nov 13, 2023
View reviewed changes
opencti-platform/opencti-front/src/private/components/data/csvMapper/CsvMapperForm.tsx Outdated
Comment on lines 66 to 68
csvMapper,
onSubmit,
}) => {
Copy link
Member

Choose a reason for hiding this comment

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

wow what happened here ? 😅

Copy link
Member

Choose a reason for hiding this comment

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

66:1 error Expected indentation of 2 spaces but found 64 @typescript-eslint/indent
you have some lint errors in drone.

Beside that this PR seems good to me 🙌

Copy link
Member Author

Choose a reason for hiding this comment

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

The format code align with the function argument position. Need to change some rules in the IDE I think.

@jpkha jpkha changed the title [Frontend/Backend] Skip line when the config give a specific char [Frontend/Backend] Skip line when the config give a specific char #4505 Nov 13, 2023
@jpkha jpkha changed the title [Frontend/Backend] Skip line when the config give a specific char #4505 [Frontend/Backend] Skip line when the config give a specific char (#4505) Nov 13, 2023
@jpkha jpkha merged commit ad08bb5 into master Nov 13, 2023
3 checks passed
@SamuelHassine SamuelHassine deleted the issue/4505-2-bis branch November 19, 2023 00:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SouadHadjiat SouadHadjiat SouadHadjiat left review comments

@Kedae Kedae Kedae left review comments

@RomuDeuxfois RomuDeuxfois RomuDeuxfois left review comments

@labo-flg labo-flg labo-flg approved these changes

Assignees

@jpkha jpkha

Labels
filigran team use to identify PR from the Filigran team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jpkha @RomuDeuxfois @SouadHadjiat @Kedae @labo-flg