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

Part import #1588

Merged
merged 58 commits into from Jun 26, 2021
Merged

Part import #1588

merged 58 commits into from Jun 26, 2021

Conversation

matmair
Copy link
Member

@matmair matmair commented May 15, 2021

Enables Import of parts building on the great additions of @eeintech in #1561.

Missing:

  • tests
    updating of parts? Maybe out of scope
  • category
  • default location

@matmair
Copy link
Member Author

matmair commented May 16, 2021

@eeintech would you mind taking a look at this? I tried making the html-implementation a bit simpler.

@matmair matmair marked this pull request as ready for review May 16, 2021 14:14
@eeintech
Copy link
Contributor

eeintech commented May 17, 2021

@matmair I pulled your branch and here are my thoughts:

  • Part import can be a "dangerous" operation, meaning not all users should be able to do it as it is too easy to make a mistake. I would advise that for database entries like parts we create an import section in the settings. Also, why not improving the import process in the admin section instead? Also, what is the use case for quickly importing parts into the database?
  • Some of the field keys are lower case while other are "titled"

image

  • The category matching needs thought, one should be able to match the PK or by name, maybe separating tree element by comma or slash? Like "Capacitor, Ceramic, 0603" will parse the category tree and try to match it to what's in the database.

  • Should we try to make some generic templates for the different steps of FileManagementFormView which can be re-used in all places?

Copy link
Contributor

@eeintech eeintech left a comment

Choose a reason for hiding this comment

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

I really like those improvements, I'm wondering if we can get them into the more generic implementation of the FileManagementFormView with associated templates so we can re-use them in all places with import option?

InvenTree/part/views.py Outdated Show resolved Hide resolved
InvenTree/part/views.py Show resolved Hide resolved
InvenTree/common/forms.py Outdated Show resolved Hide resolved
InvenTree/common/files.py Outdated Show resolved Hide resolved
InvenTree/common/views.py Show resolved Hide resolved
@matmair
Copy link
Member Author

matmair commented May 17, 2021

Part import can be a "dangerous" operation, meaning not all users should be able to do it as it is too easy to make a mistake. I would advise that for database entries like parts we create an import section in the settings. Also, why not improving the import process in the admin section instead? Also, what is the use case for quickly importing parts into the database?

The environment in which I plan to use InvenTree has multiple teams. I do not want these teams - which will add a lot of parts - to use the admin. There are a lot of options which will confuse the user. We could but a permission-flag on that function.
One of the use cases I have is in a mid-size commercial environment with a lot of R&D going on. The other is a uni-adjacent lab with a high turnover of students - which should all use the system. That is also why I invest a lot of time into translation and ease of use.

Some of the field keys are lower case while other are "titled"

image

That is something I am working on, is a result of how headers are configured and used.

The category matching needs thought, one should be able to match the PK or by name, maybe separating tree element by comma or slash? Like "Capacitor, Ceramic, 0603" will parse the category tree and try to match it to what's in the database.

I would like to keep it simple and only human-ready at first. manly see this feature used to import an extended BOM to quickly get stuff going and keep the barrier of entry low.

* Should we try to make some generic templates for the different steps of `FileManagementFormView` which can be re-used in all places?

Absolutely, I just wanted you to check how I did it first and gauge if the way I went is something you approve of. I try to not mess with others features.

@eeintech
Copy link
Contributor

The environment in which I plan to use InvenTree has multiple teams. I do not want these teams - which will add a lot of parts - to use the admin. There are a lot of options which will confuse the user. We could but a permission-flag on that function.
One of the use cases I have is in a mid-size commercial environment with a lot of R&D going on. The other is a uni-adjacent lab with a high turnover of students - which should all use the system. That is also why I invest a lot of time into translation and ease of use.

I understand your use-case. However keep in mind that we don't use InvenTree for the same thing. So for me this function should be hidden somewhere, not in clear view. Staff users have access to InvenTree global settings so I would suggest to move it there, in the Parts settings maybe. I personally don't want our team users to be able to easily import parts from a file, but all of our users have all permissions for parts. So I don't know that permission will solve that.
If you intend your use-case to expand to other sections, I would really like to see it in the settings instead. It could be a specific section too, accessible by all users deemed allowed for "database imports".

@matmair
Copy link
Member Author

matmair commented May 17, 2021

The environment in which I plan to use InvenTree has multiple teams. I do not want these teams - which will add a lot of parts - to use the admin. There are a lot of options which will confuse the user. We could but a permission-flag on that function.
One of the use cases I have is in a mid-size commercial environment with a lot of R&D going on. The other is a uni-adjacent lab with a high turnover of students - which should all use the system. That is also why I invest a lot of time into translation and ease of use.

I understand your use-case. However keep in mind that we don't use InvenTree for the same thing. So for me this function should be hidden somewhere, not in clear view. Staff users have access to InvenTree global settings so I would suggest to move it there, in the Parts settings maybe. I personally don't want our team users to be able to easily import parts from a file, but all of our users have all permissions for parts. So I don't know that permission will solve that.
If you intend your use-case to expand to other sections, I would really like to see it in the settings instead. It could be a specific section too, accessible by all users deemed allowed for "database imports".

So a permission for parts-import would solve that? The normal users would not see the button but team leads for example would. Putting stuff into settings manly means people will be asking a lot where the function is in my experience. @SchrodingersGat do you prefer a position for this?

@SchrodingersGat
Copy link
Member

I think that it makes sense to have it in the "parts settings" page - a little bit separate to the everyday views

@matmair
Copy link
Member Author

matmair commented Jun 8, 2021

@SchrodingersGat I fixed all modal-dialog points you brought up, could you confirm if there is a bug with category-matching?

@matmair matmair mentioned this pull request Jun 8, 2021
@SchrodingersGat
Copy link
Member

@matmair I'll try to have another look at this ASAP - I don't want you to think I'm ignoring this but very busy ATM !

This was referenced Jun 13, 2021
@SchrodingersGat
Copy link
Member

@matmair I've played around with this again and found a few more issues. I can enumerate these if you like.

However after some more thinking I wonder if we are approaching this the wrong way.

We already rely on the django-import-export library which does a LOT of the heavy lifting for us. It is very good at validation of fields, and the import workflow can be highly customized: https://django-import-export.readthedocs.io/en/latest/import_workflow.html

I think that we should look at leveraging django-import-export and see if we can create the specific features we are looking for. I'd extend this to the BOM import process too!

For example, the import screen in the admin interface is "most" of the way there already:

image

I would argue that the error / warning messages provided here are already very good:

  • Indication of invalid data
  • Indication of duplicate records

For the BOM importer example, we could potentially extend the functionality that exists in django-import-export to do things like:

  • Create a new part if the referenced part does not exist
  • If the part ID is not provided, lookup by part Name

The implementation here still seems rough around the edges, and I feel it still has quite a way to go in terms of data validation and edge cases (and I think this is where django-import-export excels).

What are your thoughts?

I am not sure yet if django-import-export can be operated from "outside" the admin context - this would be crucial I think?

Another option, which might actually not be too bad, is to expand general use of the admin interface itself. For operations which are seldom used (bulk imports for example) perhaps we are much better served polishing the tools that are already there, instead of recreating the wheel. This PR already feels like we are basically re-inventing what's already present in the admin interface...

Let me know what you think. @eeintech too

I appreciate that this comes after a lot of work - sorry!

@matmair
Copy link
Member Author

matmair commented Jun 17, 2021

@SchrodingersGat you are the maintainer - your call. I will respect that.
One caveat: I use ‘django-import-export’ in nearly all projects for fast backend import of existing data. As far as I am aware some of the core functions of @eeintech 's solution for PO import like easy row/column deletion and field-mapping in the wizard are not available in import-export. And having messed with it previously turns me really of from trying that any time soon.

import-export is really rigid about input-formatting and user-configurable imports could be a great value add - esp. in environments where you do not want / can't make users admins to use the admin-area import-functions or the user-base is not used to fixing a table in Excel.

Does your comment regard all wizards or just part-imports? In the latter case I would try to cherry-pick the general improvements in this PR an make a new one without the part-import-stuff.

@matmair matmair marked this pull request as draft June 17, 2021 14:28
@eeintech
Copy link
Contributor

eeintech commented Jun 17, 2021

@SchrodingersGat It's funny you brought that up, I did look into the import-export package for the purchase order importer and strongly considered it because it is already a dependency. But for all the reasons @matmair mentioned above, I thought this would take a lot more work to leverage into a "user-friendly" multi-step environment. This package is really good for admin level (where the person using it know what's happening) but the gap to being the mainstream importer was just too big. On the other hand, your custom BOM importer had all the "user-friendly" hooks in place, so I thought it made more sense to re-use what's already there.

Now for import operations altering directly the target table (import part directly alters the part table, import build directly alters the build table and so on) is basically considered as "database setup or maintenance" so not for the every user, at least in my opinion. So this is why I was pushing back not to have this feature in the main view.
That said, all the work you've done to improve the multi-step form framework is great, so that I would like to see it merged! 😄

In summary I would propose to keep the import-export for the admin section and improve it there if need to. And use our "multi-step" framework + "user-friendly" interface for all the custom importers (BOM, purchase order, sale order, etc).

@matmair
Copy link
Member Author

matmair commented Jun 17, 2021

@eeintech I would be happy to cherry-pick that stuff but will wait till there is a clear statment if the components even will be used

@matmair matmair mentioned this pull request Jun 17, 2021
@SchrodingersGat
Copy link
Member

True that django-import-export has some limitations. I am reluctant to have "two solutions" to the same problem if we can help it, but maybe we can't help it.

I've just come across this project - https://github.com/wq/django-data-wizard - which may be another interesting option. I have not yet had time to fully look into it.

I am just concerned that we are boxing ourselves into maintaining a complex data importer tool when (potentially) there's something out there that does most of the work for us (and has other people using and validating it too!)

@eeintech
Copy link
Contributor

I have looked at many projects, packages, etc. before jumping into the purchase order importer wizard but have not came across django-data-wizard. I like that it is nicely documented, provides API and can combine well with background tasks. However, it has been more than a year it hasn't been updated and I'm always reluctant to add new dependencies if the project is not actively maintained... Once Django moves on, all the "abandoned" dependencies might create more issues...
If there is a similar package out there which is still pretty well alive I would definitely be on board!

@SchrodingersGat
Copy link
Member

@eeintech you're definitely right to be concerned about the maintenance issues of the library.

I think that perhaps "rolling our own" is the best approach here after all. Given the complex interactions of the various models I think it is going to be easier to write something that takes the particular requirements into account...

I've had a more thorough look around this evening and cannot find another library that looks "perfect"...

@matmair can you please fix the conflicts that have crept in and I'll have another look

@matmair
Copy link
Member Author

matmair commented Jun 18, 2021

@SchrodingersGat should we just merge the whole thing or split it up into feature branches?

You mentioned several errors above so I think that could make merging and testing simpler.

@SchrodingersGat
Copy link
Member

I think merge the whole thing. It's pretty close to being good.

@matmair matmair marked this pull request as ready for review June 18, 2021 21:27
@matmair
Copy link
Member Author

matmair commented Jun 18, 2021

@eeintech @SchrodingersGat mind checking if I missed an issue / Bug? The merge was pretty simple but better check twice before merging.

@SchrodingersGat
Copy link
Member

@matmair sorry looks like a conflict has snuck in - please address this and I'll have a final look

@SchrodingersGat
Copy link
Member

Thanks @matmair for your great work here. Happy to merge this in now. Please submit a documentation PR when you have a chance.

@SchrodingersGat SchrodingersGat merged commit 42ed95c into inventree:master Jun 26, 2021
SchrodingersGat added a commit to inventree/inventree-docs that referenced this pull request Jun 26, 2021
@matmair matmair deleted the part-import branch June 27, 2021 12:33
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