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

Multi-step form framework + Purchase order upload file view #1561

Merged
merged 31 commits into from May 12, 2021

Conversation

eeintech
Copy link
Contributor

@eeintech eeintech commented May 7, 2021

There is a lot to digest here!

In short, this PR introduces:

  • Use of django-formtools library
  • Views for using the formtools library in InvenTree/common/views.py:
    • Generic: MultiStepFormView
    • Specific to file upload and data processing: FileManagementFormView
  • Forms for the FileManagementFormView view in InvenTree/common/forms.py
  • FileManager class to handle files containing tabular data in InvenTree/common/files.py
  • PurchaseOrderUpload view in InvenTree/order/views.py
    • Allows user to upload a purchase order file from a supplier (Digi-Key, etc), process the matching to InvenTree fields and automatically create PurchaseOrderLineItem instances
    • All templates going along with that view

When (if?) this PR get merged, it will basically fix half of #431, the other half being to try to reuse the same framework for BOM upload.

@SchrodingersGat @matmair What do you guys think?

Screenshots

Let's do it

image

Match the fields

image

Match the parts

image

Done

image

@eeintech eeintech changed the title Multi part forms framework + Purchase order upload file view Multi-step form framework + Purchase order upload file view May 7, 2021
@SchrodingersGat
Copy link
Member

I'm very much on board with improving the multi form upload process. Looks like a very reasonable approach here. I'll have to dedicate some time to a closer inspection of what you've implemented here but on the surface it looks great!

@eeintech
Copy link
Contributor Author

eeintech commented May 7, 2021

@SchrodingersGat No problem, I'm pretty excited about it too! It did take me some more effort than I thought originally but I think it was worth it 😃

@SchrodingersGat
Copy link
Member

SchrodingersGat commented May 8, 2021

I exported a simple purchase order as an example dataset, to try and re-import into an empty purchase order.

I thought this would be the "simplest" way to explore your new features but I ran into a few problems:

Some things I have noticed:

Step 3 - "Previous Step" button does not work

image

Upload option should only exist when order is pending

image

Certain fields cannot be imported

image

How do you actually select the part you wish to add to the order?

image

import field names do not match export fields

image

column alignment issues

image

blank drop-down menu

image

image

field values do not carry across correctly

image

@eeintech
Copy link
Contributor Author

eeintech commented May 8, 2021

@SchrodingersGat You ran into quite a lot of problems... sorry for that.
I may have tested with a dataset biased towards Digi-Key, I'll have to try with an exported purchase order too.
I'll fix those issues and report back!

@eeintech
Copy link
Contributor Author

eeintech commented May 8, 2021

Was able to reproduce

  • "Step 3 - "Previous Step" button does not work" and "Does not let me progress, but the drop-down is empty": I think the blocker here is the field validation failing so I need to allow clicking "Previous Step" without validation at least, or do not require fields and ignore incomplete entries during the creation of PurchaseOrderLineItem instances for the "Submit Selections" to work too
  • "Upload option should only exist when order is pending": need to show/hide the view depending on order status
  • "Certain fields cannot be imported" and "import field names do not match export fields": need to complete the list of fields based on PurchaseOrderLineItem model

Was NOT able to reproduce

  • "column alignment issues" and "field values do not carry across correctly": not sure what's happening with your dataset @SchrodingersGat ... cannot reproduce on my instance. Do you mind sharing the CSV file with me?
  • "blank drop-down menu": Are you sure the supplier for the order you exported is the same as the one used for import? It won't let you match supplier parts from other suppliers. If it is, I should get to the bottom of this if you can share your file with me.

Others

How do you actually select the part you wish to add to the order?

My intention here is to let the user match a SupplierPart by either of those two means:

  1. Matching the supplier SKU field
  2. Matching the manufacturer MPN field

Given PurchaseOrderLineItem associate to a SupplierPart, and not a Part, there is no mechanism to allow users to match a Part, or we will have to update the PurchaseOrder and PurchaseOrderLineItem models to let users to do so. But that would be outside the scope of this PR.

@eeintech
Copy link
Contributor Author

eeintech commented May 8, 2021

In the process of importing already completed purchase orders into a new one, I noticed those few things I need to fix:

  • Prevent duplicate automatic field matching

image

  • Deleting a row from the field matching step does not carry over to the part matching step (the row is still shown)
  • Quantity field in step 3 is shown with 5 digits: need to clean it up

image

  • Improve handling of exact match between SKU and MPN as one can override the other

image

Optional

  • Add a warning when some PurchaseOrderLineItem were not created because:
    • order/supplier part pair already exists
    • incomplete data from the step 3 (part matching)

This could potentially call for an another form view after step 3, which summarize the import process. Maybe call that view only when problems were found?

@matmair
Copy link
Member

matmair commented May 8, 2021

@eeintech I could reproduce "column alignment issues" - my file is attached. It seems to me the problem stems from removing a column in step 2.

PO25 - Norelem - Norelem.xls

@eeintech
Copy link
Contributor Author

eeintech commented May 8, 2021

Thanks @matmair, I'll check it out when I get a chance!

@matmair
Copy link
Member

matmair commented May 8, 2021

@eeintech I already have a solution for this problem, I found it intresting and took a shot. Should I submit a PR to your repo?

@eeintech
Copy link
Contributor Author

eeintech commented May 8, 2021

Yes please! Thanks for your help!

@SchrodingersGat
Copy link
Member

@eeintech I think I misunderstood the design intent a little, uploading data from a supplier (e.g. digikey) makes more sense here than what I did. At least I helped to shake out a few issues :)

LMK when you're ready for review again and I'll take another look.

@eeintech
Copy link
Contributor Author

eeintech commented May 10, 2021

@SchrodingersGat @matmair I've updated my list of TODOs with a bunch of fixes, want to give it another shot?

I've yet to implement reference and notes fields to this import process.

@matmair
Copy link
Member

matmair commented May 11, 2021

A few things I found:

  • importing purchase_price with a currency (like exported by norelem or InvenTree) is not working
  • xlsx does not work (is commented in code already)
  • if you remove imported fields in step2 the table-formatting starts to shift
  • duplicate quantity entries shown in step 3 if quantity-field matched in step 2

@eeintech
Copy link
Contributor Author

importing purchase_price with a currency (like exported by norelem or InvenTree) is not working

I'm currently not planning on supporting this, you can set the currency manually for all items and it defaults to the main currency from the global settings.

xlsx does not work (is commented in code already)

It works now, tablib package was out of date 😃

if you remove imported fields in step2 the table-formatting starts to shift

Umm I don't have this problem no more... did you really try the latest on this branch?

@matmair
Copy link
Member

matmair commented May 11, 2021

importing purchase_price with a currency (like exported by norelem or InvenTree) is not working

I'm currently not planning on supporting this, you can set the currency manually for all items and it defaults to the main currency from the global settings.

ok than I will take a look at it onve you are finished 😊

xlsx does not work (is commented in code already)

It works now, tablib package was out of date 😃

👍

if you remove imported fields in step2 the table-formatting starts to shift

Umm I don't have this problem no more... did you really try the latest on this branch?

I thought I did but will try right now

@eeintech
Copy link
Contributor Author

@SchrodingersGat @matmair I just completed the list of "basic" features, please go ahead and test as much as you can!
Let me know if you find anything 😄

@matmair
Copy link
Member

matmair commented May 11, 2021

@eeintech first of all great features!
I made you a screenshot of the table error and steps to reproduce, maybe it is a Firefox ( 88.1. on win10 pro) problem - although i could reproduce in edge.
Steps to reproduce:

  • load attached file
  • delete: manufacturer, MPN, id
  • you now should see the following:
    grafik
  • if you resize the window the errors should disappear

I also found that exporting a purchase-order, importing it without touching it results in an error (done with the order created with the file above)
grafik
I am not sure but that could be a problem of the export-function because opening the file in vs-code shows:
grafik

👍xlsx works fine!
a warning if an order line already exists would be cool but not absolutely needed.

@eeintech
Copy link
Contributor Author

eeintech commented May 11, 2021

@matmair Do me a favor and run inv update. Forgot to mention this. Should fix the CSV import.

@eeintech
Copy link
Contributor Author

  • load attached file
  • delete: manufacturer, MPN, id
  • you now should see the following:

I don't see it in Chrome:

image

@matmair
Copy link
Member

matmair commented May 11, 2021

Import / Export works now.

@eeintech maybe the table-problem is a Firefox thing. I wrote a few colleges and updated our test-instance - I will create a sperate issue if we can reliably reproduce it.
Damn I love Firefox but stuff like this keeps popping up more frequently in last while.

@eeintech
Copy link
Contributor Author

@matmair Ha yeah don't use Firefox, got sucked in the Google spiral... It does look like the data is aligned but the background is messed up.

If you find anything else, let me know. I had in mind to maybe tell the user if some line items were not created and reasons why. But maybe for another time...

@SchrodingersGat
Copy link
Member

Looking really good now on my end (Firefox). Certainly an improvement on the BOM importer so I'd like to see that worked on once this is merged! :)

Any further features / bugs you still plan to implement on this branch?

@matmair any other comments / notes on this?

@matmair
Copy link
Member

matmair commented May 12, 2021

@SchrodingersGat No further comments, great addition!

@eeintech
Copy link
Contributor Author

The only things missing are test units I guess. I have to check how you implemented it for the BOM import @SchrodingersGat
Maybe a separate PR?

I'm eager to get this used to the BOM import too!

@SchrodingersGat
Copy link
Member

Happy for you to submit unit testing as a separate PR, but please do :)

So all OK to merge then?

@eeintech
Copy link
Contributor Author

Sounds good, let do that. For sure I'll add unit testing as it would be a major feature when used by BOM importer.

Yes ready to go!

@SchrodingersGat SchrodingersGat merged commit 9d98ecc into inventree:master May 12, 2021
@matmair matmair mentioned this pull request May 15, 2021
3 tasks
@eeintech eeintech deleted the multi_part_forms branch July 14, 2021 19:11
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