feat: metadata import (updated for v14)

[batonac]

Here's a fresh port of the metadata import functionality to the v14 branch.

Most notably, the child table field that links Shipstation Order Item Options to "Sales Order Item" fields has been refactored considerably compared to the last PR. Instead of trying to override a "Link" field and/or use a Virtual DocType, we now have JS which pushes field labels and names to a special-purpose "Select" field directly.

The code which keeps track of custom fields to delete is also moved client side, which eliminates another "hack" using frappe.flags.

Overall, this merge request is cleaner than the v13 PR, and I feel reasonably confident that the functionality is solid. Please give it a review and let me know what you think!

[commit-lint]

Co-authored-by

• Kevin Shenk kevin@avu.nu (6604ea1)

Bug Fixes

• format files (9a2dd8c)
• add tab breaks (8b19d54)

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]

@batonac, I just tested this out locally, and I think we'll still need to address some UX points I had made on the v13 PR:

• 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 and we'll also have to validate the options import table for the field being removed.
• 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
• 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?

I'll do another pass of testing once we've answered these.

[batonac]

Hey @Alchez, thanks for the review. Sorry for not being more proactive on issues from the v13 thread...

• 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 and we'll also have to validate the options import table for the field being removed.

This is mostly implemented. On the client side, the following code captures deleted fields and stores them in the session locals:

frappe.ui.form.on("Shipstation Item Custom Field", {
  before_item_custom_fields_remove: (frm, cdt, cdn) => {
    const deleted_row = frappe.get_doc(cdt, cdn);
    if (deleted_row.fieldname) {
      locals.removed_item_custom_fields = locals.removed_item_custom_fields || [];
      locals.removed_item_custom_fields.push(deleted_row.fieldname);
    };
  },
});

On the server side, this code removes the custom fields:

# delete any removed custom fields
if removed_item_custom_fields:
	# make sure that the removed field is not in the item_custom_fields variable
	removed_item_custom_fields = [field for field in removed_item_custom_fields if field not in [f.fieldname for f in item_custom_fields]]
	for fieldname in removed_item_custom_fields:
		for dt in item_doctypes:
			if frappe.db.exists("Custom Field", {"dt": dt, "fieldname": fieldname}):
				frappe.db.delete("Custom Field", {"dt": dt, "fieldname": fieldname})

What we're missing is validating the options import table. Noted!

• 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

• 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?

I don't have strong feelings one way or another about this. In my mind, the checkbox helps to make it obvious what the second table is for, which seems like a usability perk. In other words, new custom fields might not be entirely necessary to utilize the metadata import feature, depending on the scenario, and it seems like a good idea to make that clear.

I'd be glad to just let the button go, moving the "Update Custom Fields" functionality to the "Save form" event entirely.

[batonac]

Hey @Alchez, please review again!

[Alchez]

@batonac, this looks good so far, but I've added a couple of small questions as well (also have a look at the Deepsource warnings for JS).

I've also added a PR to fix formatting, add tab breaks and other small changes in the code. Have a look and thanks!

[batonac]

Your PR is merged and I dropped the section break. Hopefully we're ready to merge now!

[batonac]

Hey @Alchez, any more thoughts on merging this? I'd love to drop my fork and re-align my client's site with your upstream branch, if possible. Thanks!