Use the taleFromDataset from the api-call service

[ThomasThelen]

I'm not sure what the real problem is for issue #523 , but I found a workaround.

The offending area seems to be the importTakeFromDataset method. I tested against old UI and backend versions and things seemed to be working as expected until this change (for example, the old compose route wasn't doing this).

My fix is to do it the same way that the compose page was doing it, using taleFromDataset in the api-call service (Example here). This is definitely less Ember-y than what was going on in the create-tale-modal, but it doesn't have that issue where the dashboard is creating a volatile record with details from the import job.

To Test:

1. Check the volatile_instance_fix dashboard branch out
2. Navigate here
3. Create and launch the Tale
4. Navigate to another page or set the filter and then reset it back to All
5. Note that there isn't a Tale for the import job

[Xarthisius]

Step 4. shows a different problem: Tale is not added to the list of objects that ember caches. As a result you get an empty view, while Tale is starting in the background, which might be confusing for users.

[ThomasThelen]

They *should be taken to the run page via this PR, but I think that we still need to refresh the view's models when the modal is closed. This is done when the filters are toggled, so it shouldn't be too hard. I'll put an issue in for it, and when 517 gets merged we can handle it. Unless we want it for 0.8.

[bodom0015]

In attempting to test this with my other PR (#517), I am no longer getting error messages/redirection happening from the Browse page after creating and launching a Tale when the instance limit has been reached.

Once #517 has been merged up, we will likely need to retrofit that error-handling here due to the obvious merge conflict.

[ThomasThelen]

@bodom0015 is that related to issues #522 and #524 ? These can probably be killed with the same stone (But if I recall correctly wasn't prioritized for 0.8).

[craig-willis]

@ThomasThelen or @bodom0015 With #517 merged I'd like to review this along with those changes. Resolving the conflict isn't clear to me -- could one of you rebase this?

[bodom0015]

@craig-willis I don't think that the desired behavior has been made clear. Is there any chance we could get together and talk about exactly what needs to change here?

I vaguely recall this being brought up as part of the Notifications discussion, where I believe this was one of our 6 or 7 use cases covered by the notification panel. While we have designed/described what this use case might look like, it has not yet been implemented in the Notifications Panel (which currently only displays notifications for Start Tale and Rebuild Tale). If we were to implement said notifications, would that eliminate the need for immediate redirection upon the modal closing?

We appear to have lumped AiWT and Compose Tale together in some places of the design, while keeping them separate in others. Is there a reason why the additional asynchronous operation needs to happen while the tale is begin created? Might it be more manageable if this API endpoint simply created a Tale, spawned a job to populate the new Tale, and then return the new Tale while the Job finishes populating it in the background? I guess I'm still confused why we need the additional Job at all in this case, as importing/registering data is already a separate task..

Is this a topic on which we need further UI/UX design? I'm not sure what exactly is our expected pattern for the somewhat typical case presented by AiWT, but this appears to be precisely the same case as "Start Tale" - we create an instance (which is immediately returned), and then poll for its status until it is complete. Unfortunately, the pattern used for "Start Tale" directly conflicts with our desire to immediately redirect the user when the modal is dismissed, as the new Tale does not yet exist. So "resolving the conflict" for this case may require us to do some design work here to further consolidate these cases into a sane pattern.

[ThomasThelen]

If I recall correctly, we lumped import into a single job because we went along with the progress bar design on the compose page (which allowed us to put the progress notifications in a single job stream).

Looking at the job in gwvolman, it looks like we should be able to create the Tale before handling any registration, and then updating the Tale once complete in a potentially separate job. I agree that we should get together and chat about this before making any changes.

[craig-willis]

@bodom0015 I may be misunderstanding something here, but my expectation is that in fixing #523 we wouldn't treat the job as a tale (i.e., do not add the job object to ember cache, do not display it on the browse page, do not redirect to job ID in place of tale ID).

We can address #524 later, since it is currently a problem in v0.7 and would require more thinking about notification handling.

Is there more to this than just not interpreting the job response object as a tale?

[ThomasThelen]

On one level, the issue is that the response from the tale/import endpoint is a job so we have no way of redirecting to the run page until the job completes (because this is the final return value). This is in contrast to the tale/ endpoint that returns a tale, which is what we get here. To clear up any confusion, this line calls the response a Tale when it's really a job.

Ember may be getting confused because we tell it to create Tale model [here]() but actually send it to the tale/import endpoint via the adapterOptions-feeding it a job model instead of Tale. This is why we get a job in the ember cache, a temporary job-tale on the browse page, and the redirect to run/job._id (since tale.id is really job.id on line 196).

On the backend though, the Tale is created. When we visit the browse page on a refresh, the Tales are retrieved, grabbing the newly created (correct) Tale and showing it in the UI. That's at least what I think is going on.

Bringing us back to the top, I think that we need tale/import to return an almost empty Tale model while a job in the background fires off and handles populating the Tale with registered data (I think this is what Mike is suggesting). This allows us to redirect to the run page with a proper Tale ID while the instance is started and registered data is added.

[craig-willis]

Thanks, @ThomasThelen. I think I'm clearer now. When we moved the Compose functionality to the Create Tale model and Browse page, we didn't bring along the import handling. I understand @bodom0015's comment now and it's clear that we need to discuss this further as it likely does require additional changes.

[ThomasThelen]

As per https://github.com/whole-tale/girder_wholetale/pull/347

we can close this since we're getting a Tale model back. Following the testing steps in 347 results in proper redirect behavior on the dashboard master branch

[Xarthisius]

Obsoleted by https://github.com/whole-tale/girder_wholetale/pull/347