-
Notifications
You must be signed in to change notification settings - Fork 0
GUNDI-4878: Field mapping creation script v1.0 #397
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,103 @@ | ||||||||||||||||||
| import copy | ||||||||||||||||||
| import logging | ||||||||||||||||||
|
|
||||||||||||||||||
| from django.core.management.base import BaseCommand | ||||||||||||||||||
| from django.db import transaction | ||||||||||||||||||
| from integrations.models import Integration, RouteConfiguration | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| DEFAULT_FIELD_MAPPING = {"default": "", "destination_field": "provider_key"} | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| class Command(BaseCommand): | ||||||||||||||||||
|
|
||||||||||||||||||
| help = "Gundi v2 field mapping creation script" | ||||||||||||||||||
|
|
||||||||||||||||||
| def add_arguments(self, parser): | ||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||
| "--connection-id", | ||||||||||||||||||
| type=str, | ||||||||||||||||||
| required=True, | ||||||||||||||||||
| help="Specify the Gundi v2 connection ID [REQUIRED]", | ||||||||||||||||||
| ) | ||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||
| "--destination-id", | ||||||||||||||||||
| type=str, | ||||||||||||||||||
| required=True, | ||||||||||||||||||
| help="Specify the Gundi v2 destination ID [REQUIRED]", | ||||||||||||||||||
| ) | ||||||||||||||||||
| parser.add_argument( | ||||||||||||||||||
| '--provider-key', | ||||||||||||||||||
| type=str, | ||||||||||||||||||
| required=True, | ||||||||||||||||||
| help='Provider key to be used in field mapping creation [REQUIRED]' | ||||||||||||||||||
|
Comment on lines
+30
to
+33
|
||||||||||||||||||
| '--provider-key', | |
| type=str, | |
| required=True, | |
| help='Provider key to be used in field mapping creation [REQUIRED]' | |
| "--provider-key", | |
| type=str, | |
| required=True, | |
| help="Provider key to be used in field mapping creation [REQUIRED]" |
vgarcia13 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Nov 20, 2025
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.
Error messages on lines 54 and 62 are written to self.stdout instead of self.stderr. Error messages should be written to stderr for proper error handling and to allow users to separate normal output from error output. Change these to self.stderr.write().
Copilot
AI
Nov 20, 2025
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.
The transaction will be rolled back on error (due to the transaction.atomic() context), but the script returns early without rolling back when integrations are not found (lines 54-55, 62-63). This creates inconsistent behavior where some errors cause rollback and others don't. Additionally, returning inside a transaction block that will be exited normally means the transaction commits successfully even though an error was reported. Consider raising an exception or using a different error handling strategy to ensure consistent transaction handling.
Copilot
AI
Nov 20, 2025
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.
[nitpick] The nested structure uses a hardcoded key "obv" without explanation of what it represents or why this specific key is used. Add a comment explaining the meaning and purpose of this key in the field mapping structure, or consider using a named constant to make the code more self-documenting.
Copilot
AI
Nov 20, 2025
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.
Using get_or_create with only the name field as the unique identifier can be problematic if multiple integrations could have the same route name pattern. This could lead to unintended sharing of RouteConfiguration objects between different integrations. Consider adding a unique constraint or using the integration ID in a more unique way (e.g., as part of a unique_together constraint with name, or using update_or_create with a more specific lookup).
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.
[nitpick] The help text for this command is generic and doesn't clearly explain what the script does or when it should be used. Consider adding a more descriptive help text that explains the purpose of creating field mappings between integrations and what the expected outcome is. For example: "Creates or updates field mapping configuration between a source integration and destination integration using a provider key."