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

Refactor stock item test result form for API #3143

Merged

Conversation

SchrodingersGat
Copy link
Member

@SchrodingersGat SchrodingersGat commented Jun 6, 2022

Ref: #2253

@matmair
Copy link
Member

matmair commented Jun 6, 2022

@SchrodingersGat have you tested the performance implications if a lot of items are deleted at once?

@SchrodingersGat
Copy link
Member Author

It's not "too bad", if the user has a very large number of test results to delete, they'll be presented with the spinny wheel until they're all done.

It is the same question when we are using the API to delete a lot of stuff at once. The current solution (which is being rolled out to all the "multi delete" actions) is to sequentially send API requests to delete the items one at a time.

Advantages

  • No "new" API endpoints are required
  • Simplistic and uses code we already have
  • Sequential operations prevent the server from being "overloaded" with requests and failing some of them

Disadvantages

  • Non atomic (some of the operations might fail, but the others are not rolled back)
  • Speed issues (a single API request and DB hit to delete multiple things at once is obviously faster)

Currently leaning heavily towards simplicity. This refactor has removed a lot of old crusty "delete" views which were 90% boilerplate. If there is another way of tackling this problem which gets around the disadvantages listed above, I'd be keen to hear it :)

@matmair
Copy link
Member

matmair commented Jun 6, 2022

@SchrodingersGat I understand the approach but would like to point out that the non-atomic ness can lead to interesting side effects as results can change for multiple seconds - I would prefer something more atomic. Also this moves business logic to the frontend. So if we want to have the same in the app we need to replicate the logic there too.

An alternative solution would be to enable a delete action on the list endpoints - we already have endpoints that take parts[] or items[] as options (for printing for example).

@SchrodingersGat
Copy link
Member Author

SchrodingersGat commented Jun 6, 2022

An alternative solution would be to enable a delete action on the list endpoints - we already have endpoints that take parts[] or items[] as options (for printing for example).

I like that idea a lot. We don't need to add multiple new endpoints, just make a mixin class.

I'll merge this PR in now, but will soon launch a new PR to refactor the existing "multi delete" functions to use the list endpoint.

Atomicity and speediness are huge advantages to your suggested approach.

@SchrodingersGat SchrodingersGat merged commit ea83d4b into inventree:master Jun 6, 2022
@SchrodingersGat SchrodingersGat deleted the stock-test-data-delete branch June 6, 2022 11:27
@matmair
Copy link
Member

matmair commented Jun 6, 2022

@SchrodingersGat when the mixed n is created we could maybe standardize on using items as the argument for one or more items, not items, parts, etc. Makes it easier to code and keeps all calls similar.

@SchrodingersGat
Copy link
Member Author

100% was thinking the same thing :)

@matmair
Copy link
Member

matmair commented Jun 6, 2022

100% was thinking the same thing :)

Will be breaking thought. Good to test the new release process.

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.

2 participants