search

whole-tale / dashboard

Whole Tale Dashboard
http://wholetale.org
MIT License
7 stars 2 forks source link

Run button fix #530

Closed ThomasThelen closed 4 years ago

ThomasThelen commented 5 years ago

Bug fix for issue #528

The bottom line bug was that we weren't checking for status 0 on the instance object. There was an edge case where the instance wasn't defined on the Tale object when coming from newly created Tales so some code changes were made in that area.

To Test

Refer to #528

  1. Create and launch a Tale
  2. Note the Stop button
  3. Wait for the Tale to finish launching
  4. Stop the Tale
  5. Navigate to Browse'
  6. Launch the Tale
  7. Note the Stop button

Create Tale Modal Changes

In other places in the codebase we use api-call.startTale to start Tales. This handles instance creation and returns the instance. I changed this component to use startTale and then set the instance property on the Tale to the new one (this is important).

View Route Changes

We were returning a Tale object that we construct with api-call.fetchTaleData. I'm not 100% sure why we were using this, but not using it didn't seem to break anything. This method attaches an instance to the Tale object. One issue that I ran across was that the instance wasn't yet defined on the Tale object by the time we got to the Run page. To get around this, I set the instance in the Create Tale Modal after the call to startTale (matching what's done in other places). Additionally I just return the actual Tale object from the Store rather than the piecewise constructed Tale we were returning before.

The currentInstanceId in the local storage was getting set to the Tale ID so I took this logic out and moved it to the init method in the Run page. This led to me removing the code referencing hasSelectedTaleInstance, which was an artifact left over when we could select instances on the right hand side of the UI. I left the currentInstanceId in because it gets used elsewhere in the dashboard.

Run Page Changes

We were missing a check for instances that had a state of 0 (which is starting). I added a check for this in the template.

What Happens When You Stop a Starting Instance?

When a user clicks the big Stop button, stopTale in the component gets called. This method ends up calling stopTale in the api-call.js file. Right now we aren't doing anything when the instance status is 0 (Starting) or 2 (Failed). Is this something we want to fix in 0.8?

Notes on Commit: "Remove references to currentInstanceId"

While working on the view route of the run page, I moved a line of code that set currentInstanceId to the component, which broke things on a page refresh. This local storage variable was used in a previous version of the dashboard where the user could select a list of running Tales on the right hand side of the UI. The quickest fix was to just remove all references to currentInstanceId. This included

  1. Deleting app/components/ui/tale-instances We no longer use this component in the dashboard
  2. Removing a number of commented out references
  3. Changing the check in app/components/ui/tale-tabs-selector/template.hbs I'm pretty sure this is making sure that the embedded iframe instance is the one that was selected on the right hand side list (that no longer exists). The outer 'and' statement should now read "if there's an instance and it has an iframe"
ThomasThelen commented 4 years ago

I have a little blurb in the PR about what happens when you click Stop (you get the error mentioned above). I would prefer we handle the stop logic in another PR; from the comment it looks like we need to wait until the instance is fully created, and then delete it. Right now it attempts to delete the instance, which hasn't been fully created.