-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
BOM import: add field selection for item reference (ID/IPN/Name) #10268
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: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for inventree-web-pui-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@TehDmitry this is an interesting idea, but your approach is very much hard-wired for "part" import. The importer works for many different models, and so the approach should be generic too. I would suggest that to make this generic, we add a list of possible "Import ID" fields to the underlying import model class. So, for the "Part" class, you might add: class Part(...):
def import_id_fields(self):
"""Return a list of potential ID fields for import."""
return ['pk', 'IPN', 'name']
If this method is not specified, then the importer just uses the 'pk' field (as a default). The import process then matches the provided field values, to the "names" of the fields (which are translated already, so we don't need to replicate that). Then, the import system exposes the available import field types for any "foreign key relation" to the frontend. I like the frontend implementation, that's a clean way of handling this. Overall, this is a really nice enhancement - we just need to make it generic! |
src/frontend/src/components/importer/ImporterColumnSelector.tsx
Outdated
Show resolved
Hide resolved
src/frontend/src/components/importer/ImporterColumnSelector.tsx
Outdated
Show resolved
Hide resolved
src/frontend/src/components/importer/ImporterColumnSelector.tsx
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (17.64%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #10268 +/- ##
==========================================
- Coverage 87.40% 86.19% -1.22%
==========================================
Files 1266 1266
Lines 56455 57830 +1375
Branches 2061 2064 +3
==========================================
+ Hits 49345 49847 +502
- Misses 6603 7475 +872
- Partials 507 508 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| commit=False, | ||
| ) | ||
|
|
||
| for key in row.data: |
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.
Looks like you still have some hard-coded fields here - the "import_as" fields should not have to be called out by name.
| @staticmethod | ||
| def import_as_fields(): | ||
| """Return a list of potential ID fields for import.""" | ||
| return ['id', 'IPN', 'name'] |
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.
Nice, this is a simple way to specify the import-as options
| ); | ||
| } | ||
|
|
||
| function ImporterPartImportAsSelect({ |
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.
This should be ImporterImportAsSelect - it is not specific to importing parts, it should be generic across any type of import
|
@TehDmitry have you had any chance to review this further? |
|
Any updates on this? We need this functionality, maybe I can help? |

#9248, #9382 and partly #7574
inspired by #2349
edit matmair:
Closes #9248
Closes #7574