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

[BUU] Avoid validating all records when saving #12503

Closed
dacook opened this issue May 22, 2024 · 4 comments · Fixed by #12515
Closed

[BUU] Avoid validating all records when saving #12503

dacook opened this issue May 22, 2024 · 4 comments · Fixed by #12515
Assignees

Comments

@dacook
Copy link
Member

dacook commented May 22, 2024

What we should change and why (this is tech debt)

On the old Bulk Edit Products screen (/admin/products with feature toggle admin_style_v3 OFF), when you edit some rows and click save, only the edited data are sent to the server, validated and saved.

On the new screen (/admin/products with feature toggle admin_style_v3 ON), it currently submits all data, and validates and saves everything.
This is less efficient and probably slower. It can also cause confusion when saving, if you have an invalid record in view (corrupt data).

Context

This is demonstrated in #12486 (comment)

@dacook
Copy link
Member Author

dacook commented May 22, 2024

  1. Can we trim un-changed fields from bulk_update? But does it make much difference? It might help when dealing with large datasets
    • Unable to do it with SR as there isn't a necessary hook
    • Unable to do it with a standard form submission, you need to modify the DOM before submitting, so
    • Maybe set unchanged fields to disabled (and ensure the style doesn't look wrong).
    • Alternatively, write custom JS to build the payload of changed fields and post with AJAX.
  2. Can we trim un-changed rows from bulk_update? No because the model set still needs to reload them all for the current architecture. Better just to strip un-changed fields, keeping IDs.
    • it would be helpful to update ProductSet to only validate changed records. Because some products might be invalid due to bad data, and show an error message even if you didn't try to update it.

@dacook
Copy link
Member Author

dacook commented May 22, 2024

I think that setting unchanged fields to disabled would be quite effective, and mostly safe because we re-render the view on success or failure. Except in the case of network failure. I have to admit I'm not sure how we currently handle network failure, but ideally it should be able to revert and let you try again.

The simplest, and safest option is to update the ProductSet to skip over unchanged records.

@isidzukuri
Copy link
Contributor

ive tried to save only changed products #12515. But still, variants should be saved here https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/services/sets/product_set.rb#L107 . Simply put there variant.changed? would not help because it does not take into account assigned attributes of associations. I haven't found suitable method in Spree::Variant interface.
So it looks so-so....
I would vote for sending from view to BE only changed data if somebody asked me. Modern approaches used by React seems to be working ok for such pages. Less bandwidth is used, fever request, less load on server.... I can assume this is a new implementation since its hidden by feature flag. Maybe it would be useful to redesign workflow slightly and lay down fundamentals which can be reused somewhere in the project later.
From the other hand its admin page. Changes by user will be made here once for a while. It shouldn't be a high load point of application. Im not sure if it worth time spent on optimization.

@dacook
Copy link
Member Author

dacook commented May 27, 2024

Thanks @isidzukuri!
I had a look, and I think you were very close. It turned out that variant.changed? didn't work because of some custom OFN code. I've added a fix to your branch, and it looks good now.

I would vote for sending from view to BE only changed data

I agree that's the ideal. However I think we can avoid that for now. The screen is still quite new and we want to ensure the basic features are working well before changing too much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants