-
Notifications
You must be signed in to change notification settings - Fork 799
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] prevent delete CSV mappers if used by CSV ingesters (#6757) #6891
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6891 +/- ##
==========================================
- Coverage 67.42% 67.41% -0.01%
==========================================
Files 561 561
Lines 68510 68533 +23
Branches 5713 5713
==========================================
+ Hits 46191 46200 +9
- Misses 22319 22333 +14 ☔ View full report in Codecov by Sentry. |
3ea91d4
to
ba03a89
Compare
Back to draft: need to add a migration script to fix ingesters that might have lost their mapper. |
ba03a89
to
3c7a1aa
Compare
3c7a1aa
to
08c5fa5
Compare
@@ -657,7 +657,7 @@ | |||
"Do you want to delete this country?": "Souhaitez-vous supprimer ce pays ?", | |||
"Do you want to delete this course of action?": "Souhaitez-vous supprimer cette conduite à suivre ?", | |||
"Do you want to delete this CSV ingester?": "Voulez-vous supprimer cet ingéreur CSV ?", | |||
"Do you want to delete this CSV mapper ?": "Voulez-vous supprimer ce mappage CSV ?", | |||
"Do you want to delete this CSV mapper?": "Voulez-vous supprimer ce mappage CSV ?", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to the PR, but we would need at some point to review the auto translations. In french, CSV mapper has currently 3 different translations "mappage CSV", "mappeur CSV", "mapping CSV"
logApp.info(`${message} > started`); | ||
|
||
const allIngesters = await listAllEntities(context, SYSTEM_USER, [ENTITY_TYPE_INGESTION_CSV]); | ||
const mappersUsedIds = allIngesters.map((ingester) => ingester.csv_mapper_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor suggestion : maybe add a condition on allIngesters length before this line :
if (allIngesters.length > 0)
otherwise, no need to continue.
const ingesters = await listAllEntities<BasicStoreEntityIngestionCsv>(context, user, [ENTITY_TYPE_INGESTION_CSV], opts); | ||
// prevent deletion if an ingester uses the mapper | ||
if (ingesters.length > 0) { | ||
throw FunctionalError('Cannot delete this CSV Mapper: it is used by one or more IngestionCsv ingester(s)', { id: csvMapperId }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe "CSV Feed" instead of "IngestionCsv" ? since it's the term we use in the platform
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally ✅
- migration works as expected, deleted csv feed without csv mapper
- can't delete anymore csv mappers if used by a csv feed
Proposed changes
IngestionCsvCreation
Related issues
Checklist