feat: metadata import

[batonac]

This code closes #52

They say a picture is worth a thousand words, so here goes:

[commit-lint]

Contributors

batonac, Alchez

Commit-Lint commands

You can trigger Commit-Lint actions by commenting on this PR:

• @Commit-Lint merge patch will merge dependabot PR on "patch" versions (X.X.Y - Y change)
• @Commit-Lint merge minor will merge dependabot PR on "minor" versions (X.Y.Y - Y change)
• @Commit-Lint merge major will merge dependabot PR on "major" versions (Y.Y.Y - Y change)
• @Commit-Lint merge disable will desactivate merge dependabot PR
• @Commit-Lint review will approve dependabot PR
• @Commit-Lint stop review will stop approve dependabot PR

[Alchez]

Hey @batonac, thanks for sending up a contribution! I have some general design notes/questions though:

• I see that you've set it up so that every time a new order is fetched, the system will start populating the "Shipstation Options Import" table.
  • Can we request Shipstation to give us all product info with options at once? Rather than having to wait until a product is imported via an order?
  • Since this can happen async after the first import, should there be a way (a flag to allow setting up notifications, maybe) to notify users that a new option requires an SO field?
• In your screenshot, I see that you've mapped two options to the same SO field.
  • Are the fields coming from different stores, or have alternate names?
  • Shouldn't the default behaviour be a 1:1 map? Which option takes precedence between the two if both exist?
  • If the expected behaviour is a 1:1 map, then I don't see a real use of the custom fields table. Maybe the two can be merged?
• The custom fields table and create/update custom fields button seem out-of-place for this doctype.
  • This behaviour already exists in Customize Form, and custom fields should ideally be handled there instead.
  • Suggestion: If we still want to handle it here, I think a better UX might be:
    • add a button to show a prompt (as a table, similar to "Update Items" in Sales Order) that lets users view which option fields are missing
    • let user decide what each field needs to be in the prompt's table
    • add action to have the system create the missing custom fields
    • let the user map each option in the table from a dropdown of fields
• For users that are also using the accounting module, I think it might be good to setup the fields in the Sales Invoice and Delivery Note doctypes as well.

[batonac]

Hey @Alchez, thanks for the review! Here are my responses:

• I see that you've set it up so that every time a new order is fetched, the system will start populating the "Shipstation Options Import" table.

I'm not totally following you here... the system does query the "Shipstation Options Import" table when an order is processed, to see if any of the order items have matching metadata/options. It will import an option from Shipstation only if there's a pre-configured match on that table. It never does pull a list of product options from Shipstation, at present. It assumes that you will know the name of the options you want to import and will pre-specify them on the "Shipstation Options Import" table.

• Can we request Shipstation to give us all product info with options at once? Rather than having to wait until a product is imported via an order?

• Since this can happen async after the first import, should there be a way (a flag to allow setting up notifications, maybe) to notify users that a new option requires an SO field?

• In your screenshot, I see that you've mapped two options to the same SO field.

  • Are the fields coming from different stores, or have alternate names?

I can only speak of Shipstation Woocommerce integration, and my own client's scenario. We are using custom fields/option on our Woocommerce setup, and as such there would be no realistic way (that I know of), to query all the possible options via API ahead of time. The custom options are fed into the metadata on each Woocommerce Order Item, and then passed through to Shipstation as order item options. In a more standardized scenario with parent and variant items, this might be more "doable", though I'm not experienced enough with Shipstation to know how well it supports importing and syncing product variations and attributes across various products.

To be even more specific, you, understandably, are probably thinking of "Variable Products", but what we're using is actually Gravity Forms Product Add-ons. This is obviously specific to our e-commerce platform and not Shipstation per-se, but it illustrates why I took the approach I did.

Not everyone will have such an unstructured approach, but I assume it won't be totally uncommon. Free-formed product options are definitely a thing. This explains the following as well:

• Shouldn't the default behaviour be a 1:1 map? Which option takes precedence between the two if both exist?
• If the expected behaviour is a 1:1 map, then I don't see a real use of the custom fields table. Maybe the two can be merged?

What's actually happening here is that the product options are not completely standardized on the e-commerce side. Thus, this isn't about one product option taking precedent over another, but in being able to scrape options from differently structured products and funnel them into a single/consolidated field in ERPNext. In a perfect world, there would be a 1:1 map, but in my experience, that's expecting a bit too much... (I'm not aware of any widely-adopted standardized schema for product options). In this scenario, we have a wholesale form that names the options differently than our retail form. We could fix/change this if need be, but I would expect some of the same kind of scenarios almost anywhere that local/product-specific options can exist.

• The custom fields table and create/update custom fields button seem out-of-place for this doctype.

  • This behaviour already exists in Customize Form, and custom fields should ideally be handled there instead.

  • Suggestion: If we still want to handle it here, I think a better UX might be:

    • add a button to show a prompt (as a table, similar to "Update Items" in Sales Order) that lets users view which option fields are missing
    • let user decide what each field needs to be in the prompt's table
    • add action to have the system create the missing custom fields
    • let the user map each option in the table from a dropdown of fields

I could certainly embrace this approach. As I'm envisioning this solution, the sync engine would add a new entry to the "Shipstation Options Import" table any time it encounters a newly-named option, and let the user select from existing fields from the Order Item table, or create new ones.

Something like this?

ShipStation Option Name	Sales Order Item Field	Create Field	Label	Type	Options
Name (text/data)	(Dropdown of Fields)	(checkbox to enable fields to the right)	Label (text/data)	(Dropdown of Field Types)	Options (text/data)

I'm assuming this would still allow for a break from the "1:1 map" (from the above discussion), is that right?

• For users that are also using the accounting module, I think it might be good to setup the fields in the Sales Invoice and Delivery Note doctypes as well.

Interesting. I'm seeing that "make_sales_invoice" is being used from erpnext.selling.doctype.sales_order.sales_order to make the sales invoice, and "make_delivery_note" from erpnext.accounts.doctype.sales_invoice.sales_invoice for the delivery notes. I didn't study either extensively, but are you thinking that it would be as simple as ensuring that all the custom fields are replicated on both those doctypes to allow for the data sync?

[Alchez]

Hey @batonac,

I'm not totally following you here... the system does query the "Shipstation Options Import" table when an order is processed, to see if any of the order items have matching metadata/options. It will import an option from Shipstation only if there's a pre-configured match on that table. It never does pull a list of product options from Shipstation, at present. It assumes that you will know the name of the options you want to import and will pre-specify them on the "Shipstation Options Import" table.

Oh sorry, I might have missed some detail in this part of the code 😅

We are using custom fields/option on our Woocommerce setup, and as such there would be no realistic way (that I know of), to query all the possible options via API ahead of time.

Yeah, I just browsed through the API, and the item options only seem to be available while importing an order. Oh well.

[...] this isn't about one product option taking precedent over another, but in being able to scrape options from differently structured products and funnel them into a single/consolidated field in ERPNext. In a perfect world, there would be a 1:1 map, but in my experience, that's expecting a bit too much... (I'm not aware of any widely-adopted standardized schema for product options). In this scenario, we have a wholesale form that names the options differently than our retail form.

Okay, I understand now. I had assumed that the options were coming from the same source, but it makes more sense that they're mapping different contexts.

As I'm envisioning this solution, the sync engine would add a new entry to the "Shipstation Options Import" table any time it encounters a newly-named option, and let the user select from existing fields from the Order Item table, or create new ones.

Something like this?

ShipStation Option Name	Sales Order Item Field	Create Field	Label	Type	Options
Name (text/data)	(Dropdown of Fields)	(checkbox to enable fields to the right)	Label (text/data)	(Dropdown of Field Types)	Options (text/data)

I think you've got it. My only question here would be: is there any additional filters or controls you think a user would want on these new fields? Such as read-only, hidden, in grid view, etc.?

If the answer is yes, then I think it would be better to handle it in Customize Form since it allows for larger control over the fields (I think this will also come down to the level of permissions, because adding Custom Fields is a very restricted action). But either way, this could still be a "Quick Entry" way of adding the fields and then modifying them manually later.

I'm assuming this would still allow for a break from the "1:1 map" (from the above discussion), is that right?

Yes, if we take up the prompt approach, then we should be able to arbitrarily map multiple options to the same fields.

[...] are you thinking that it would be as simple as ensuring that all the custom fields are replicated on both those doctypes to allow for the data sync?

Internally, both those functions use a mapper function, which can implicitly map data across doctypes if the fieldnames are the same, so you should be able to just the add the field and be done.

[batonac]

From my perspective, there are two compelling reasons to retain custom-field creation functionality in this app:

1. There are an immense number of fields in the doctypes in question, making the "customize form" route a tad bit overwhelming. This app already creates its own set of custom fields already, and it makes sense to me to allow for a few more that remain bundled in the same area as the existing shipstation_integration custom fields.
2. As per the above discussion, we are now looking at replicating these fields across three doctypes. This would be both tedious and open the door to quite a bit of user-entry error if we expect the user to replicate/maintain these fields and options verbatim across these three doctypes.

That said, if we are indeed steering away from a 1:1 map, then I'm leaning back towards the initial concept here of two separate child tables for managing custom field creation and shipstation option mapping. I believe the experience could be improved by eliminating the side-by-side view, and hiding the custom field creation behind a conditional checkbox. The custom field creation table could be built out with all the basic field option inputs, as needed.

Here's what I'm envisioning currently (with an example in the bottom row):

Metadata Import

Shipstation Options Import

Shipstation Option Name	ERPNext Item Field
Name (text/data)	(dropdown of fields)
Fabric	Shipstation Fabric   ▼

☑ Create Custom Item Fields

Order Item Custom Fields (on Sales Order Item, Sales Invoice Item, and Delivery Note Item Child Tables)

Label	Type	Name	Options	Mandatory	Read Only	Hidden
(text/data)	(dropdown)	(text/data)	(small text)	(check)	(check)	(check)
Shipstation Fabric	Link	shipstation_fabric	Item	☑	☐	☐

How would you feel about this approach?

[Alchez]

From my perspective, there are two compelling reasons to retain custom-field creation functionality in this app

I think it's good UX to allow custom fields to be made in the context of the Shipstation, and I agree with both of your points. My concerns were mostly around duplication and maintenance of features, but that could be a small con against the benefit of being able to create the fields in-place.

That said, if we are indeed steering away from a 1:1 map, then I'm leaning back towards the initial concept here of two separate child tables for managing custom field creation and shipstation option mapping. I believe the experience could be improved by eliminating the side-by-side view, and hiding the custom field creation behind a conditional checkbox. The custom field creation table could be built out with all the basic field option inputs, as needed.

Yeah, this design is good. I would just suggest keeping the "Create Custom Item Fields" checkbox as a button from your initial design. That would show the prompt and also let you apply permission levels incase someone needed to put that button behind a role.

Speaking of which, we should consider the following:

• keep the status quo, and assume only users with the "System Manager" will be adding the fields, since they're the only non-admin users that can create Custom Fields
• on the settings page, suggest using "Role Permission Manager" to allow more roles to create Custom Fields
• create a new, standard Shipstation Manager role and allow it to create Custom Fields
• simply ignore permissions when making the Custom Fields (potentially dangerous).

[batonac]

Hey @Alchez, I believe the layout and functionality changes we discussed are all committed and ready to merge.

• The 'Shipstation Options Import' table now has a dynamic link to all available (Sales Order Item) fields in the 'Sales Order Item Field' field
• The 'Sales Order Item Custom Fields' table is now in it's own row and hidden behind a new 'Define Custom Fields for Sales Order Items' checkbox. There are a few more option available to define these fields as well.
• The 'Create/Update Custom Fields' button will now create custom fields, in order, following the existing 'shipstation_item_notes' field, on the "Delivery Note Item", "Sales Order Item", and "Sales Invoice Item" doctypes.
• The order importer will first check for an available assignment for an Item Option, import it if a field is assigned, and create a new entry in the 'Shipstation Options Import' table if the ShipStation Option Name isn't already present (it might be good to refresh the page automatically if this happens, but I didn't bother).
• A new hours_to_fetch field replaces the hardcoded last_order_datetime variable.

I believe the only thing we discussed that wasn't touched is the permissions. Since the Shipstation Settings doctype is already limited to "System Manager" or greater, I figured that can be refined down the road if it's necessary to open this up a bit more.

Please review!

[Alchez]

@batonac, hey sorry, I haven't had a chance recently to get to this. I haven't done a functional review of the feature yet, but I've added a few comments on the code.

I'll try and get to the functional review sometime in the next 2 weeks.

Can you also please have a look at the Deepsource linter issues for styling?

[Alchez]

Can you add a comment here for why we're doing this?

[batonac]

you have a keen eye @Alchez. both of these code comments are related to the same issue.

I consider the use of a link control for the sales_order_item_field to be a really slick solution, but allowing both DocFields and Custom Field to be selected interchangeably requires a custom query, which throws Frappe into a tizzy when trying to validate the links, even with a "Dynamic Link" populated control.

This was my workaround. If there's a better way to handle this, I'd be glad to know how.

[Alchez]

@batonac, I see what you're trying to do. I think we could get away with replacing the Link control with a Select control, which would still let you populate a list to choose from as well as avoid any link validation issues.

The only downside might be the UX, since you would only be able to show the fieldnames in the list, and not the label (unless you add some special processing to separate the two, but that might not be worth it).

I think Link -> Select is a good enough trade-off.

[Alchez]

Can you add a comment for adding this flag?

[Alchez]

@batonac

Okay, I tested out the changes, and I think it's almost there! Here's my notes on the feature's UX:

• There is a clear way to create and update custom fields, but should the page also allow you to delete custom fields from the page (ex. old options that are no longer relevant; a counter-argument could be keep that data for legacy orders)? If this is a valid scenario, this could happen when a row is deleted from the custom fields table.
• It is currently possible to add a row and immediately click on the "Create/Update Custom Fields" button. This successfully generates the custom fields in the doctypes, but if the user then forgets to save the Shipstation Settings record itself, that custom Shipstation field information is lost.
  • I think either the button should disappear or throw a warning when the form is in an unsaved state
• It is currently possible to delete rows from the custom fields table. Should we allow that? If so, would that also mean deleting the custom fields themselves? If both answers are yes, we'll also have to validate the options import table for the field being removed.
• I'm unsure about the conditional checkbox design. It's being used to allow/prevent creating custom fields, but that's already the role of the button. Seems redundant. Maybe we just always show the custom fields table?
• The general wording in the Metadata Import section focuses on Sales Order, but I think we should make it apparent that these are also applied for Sales Invoice and Delivery Note.
• Can you rename the table doctypes:
  • "Shipstation Options Import" -> "Shipstation Option"
  • "Shipstation Item Custom Fields" -> "Shipstation Item Custom Field" (singular)

[Alchez]

@batonac, just an FYI: we've renamed the default branch from master to main-v13 to ensure compatibility with Frappe's versioning. If your PR is meant for a specific version (v13 or v14), please change it accordingly.

Is there anything left to be done in this PR by you, or should I give it another review?

[batonac]

Hey @Alchez, that sounds good. v14 compatibility of this app was the last thing I was waiting for before moving my client to that version. In the meantime, I'll try to get the field link fields patched up, and then submit for another review.

[batonac]

Hey @Alchez!

So I just took another stab at the Item Field Linking issue, this time by implementing a Virtual DocType for listing/linking the standard and custom fields available in the Sales Order Item Doctype.

I had hoped this would provide an extremely elegant and robust solution, including cleaning up code and dropping workarounds, but alas, it seems that Virtual DocTypes are not very well-supported before Version 14, so the custom item_field custom query and ignore_link_validation remains, at least on this branch. I believe that most of that can be dropped when rebasing against Version 14.

I'll await your direction at this point. Is this okay to merge in the v13 branch and then rebase against v14 as a separate PR, or should I rebase this entire PR against that version/branch?

[Alchez]

@batonac

Ideally I'd like to avoid having to maintain two different versions of the feature code. Do you need this feature for v13?

• If yes, then this PR can stay on v13.
• If no, then rebasing this PR to v14 would be best.

In either case, I can't guarantee code maintenance, since I haven't worked with Virtual Doctypes much (yet), but PRs are welcome and I wouldn't mind reviewing them.

As for your new changes, I'm working on different clients/projects, so it might be sometime before I can get around to properly reviewing it.

PS: can you have a look at the Deepsource Python lint errors? I think the JS one can be ignored for now. If you're making this to v14, you can also use the pre-commit hooks defined in the repo.

[batonac]

Sounds good @Alchez. I'll rebase this against v14 and check out the DeepSource Python errors.

As for the Virtual DocType, I don't believe it's quite as "scary" as it seems on the onset. The heart of it is a very simple DocType definition which bypasses the need for a dedicated table and simply operates based on a dedicated get_list function which returns the following dataset:

{"name": f.fieldname, "fieldname": f.fieldname, "label": f.label}
for f in frappe.get_meta("Sales Order Item").fields
if f.fieldtype in ["Data", "Text", "Small Text", "Link", "Select"]

This should allow for "properly" linking to all available fields in the Sales Order Item DocType (standard and custom) without disabling link validation, creating any other custom query override, or (alternatively) creating a custom function to populate a select field. I trust that, upon review, you will appreciate the elegance of this approach.