qri-io / desktop

Qri Desktop
https://qri.io/desktop
GNU General Public License v3.0
67 stars 14 forks source link

feat(Collection): bulk remove datasets #644

Closed uhLeeshUh closed 4 years ago

uhLeeshUh commented 4 years ago

Closes #636

b5 commented 4 years ago

I think the answer to your question is to modify the signature of the function passed as the OnSubmit prop to the RemoveDataset, to make it more opaque, going from this:

export interface RemoveDatasetModal {
   // ..
   onSubmit: (...args: any) => any
}

to this:

export interface RemoveDatasetModal {
   // ..
   onSubmit: () => Promise<AnyAction>
}

you're already doing this wrapping inside the handleSubmit function of RemoveDataset. By moving it out and requiring callers pass an opaque function that returns a promise, you can push configuration up to the call site while retaining the requirement that the onSubmit prop return a promise so you can await onSubmit()

b5 commented 4 years ago

As for the principle problem you were having of a single checkbox bulk action, a set of 1 is still a set, and we should consider it a bulk action even though it's a single dataset. The fact that the same modal handles both singular and bulk (set-oriented) actions is a coincidence

uhLeeshUh commented 4 years ago

@b5 along the lines of treating a set of 1 as still a set, do you have advice for differentiating how to correctly pass args to the onSubmit function here? The problem I've encountered is I don't have a reliable way of knowing if the single dataset to be removed was sent to the modal via the kabob menu or the bulk action button (this check of just "is there only one dataset" isn't sufficient). If via the kabob, I need to pass the args as a list to the onSubmit fn to match up with removeDatasetAndFetch, but if via the bulk action, then I treat it as the "set of one" approach and pass in an array to satisfy removeDatasetsAndFetch.

Unless, when you said "set of 1" did you mean we should reconcile the params for removeDatasetAndFetch and removeDatasetsAndFetch to always accept an array? And for the former it will always just hold a single object. That was the first solution I thought of but wasn't sure if it was kosher given that it deviates from the normal function signature we use for actions

b5 commented 4 years ago

I'm saying build the onSubmit function before this handler is ever called, and pass it to RemoveDataset. You've got the problem figured out:

The problem I've encountered is I don't have a reliable way of knowing if the single dataset to be removed was sent to the modal via the kabob menu or the bulk action button

That's because this info is owned by the caller. So keep the RemoveDataset modal dumb and make it's onSubmit prop a function that the caller has to construct.

uhLeeshUh commented 4 years ago

Ahhh gotcha. The only thing we'll need from the handler is the keepFiles value then but we can close over the other values in the correct format. Ok cool! I'll give that a go now

ramfox commented 4 years ago

Another note, I don't think your tests will pass until the rename pr is merged, I had to do a lot of finessing on tests that relied on elements that had changed! But the tests that you added definitely work.

uhLeeshUh commented 4 years ago

Oh yeah @ramfox nice idea to simplify the passed action type to VersionInfo -- I was finding it hard to keep track of those two similar types and think VersionInfo as a used pattern for all our actions is great