search

whole-tale / dashboard

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

Added "Connect Additional Accounts" view #563

Closed bodom0015 closed 4 years ago

bodom0015 commented 4 years ago

Problem

We need a new view for the "Connect Additional Accounts" functionality.

Fixes #559
Fixes #560 Fixes #561

Approach

Once the API is live, real data can be wired up. This could either be done as a part of this PR, or as a subsequent one.

Demonstration

http://recordit.co/YNyn1S1JVj

How to Test

Prerequisite: must run alongside https://github.com/whole-tale/girder_wholetale/pull/364

  1. Checkout and run this branch locally, rebuild the dashboard
  2. Login to the WholeTale Dashboard
    • You should see the navbar has a new "user" icon at the top-right
  3. Click the "user" icon
    • You should see a dropdown expand
    • The dropdown should contain the user's first/last name and gravatar image
    • The dropdown should provide a link to the Settings page
    • The dropdown should provide an option to Logout
  4. Choose the "Settings" nav link
  5. Click "Connect Account" to the right of Zenodo
    • You should see a modal appear allowing you to choose a resource server and enter an API key
  6. Choose a resource server, enter an API key, then click Connect
    • The modal should close
    • You should see a new "Authorized on" entry appear beneath Zenodo
    • You should see a red "Disconnect" button beside the "Authorized on" line
  7. Click the red Disconnect button
    • You should be greeted with a confirmation modal
  8. Confirm the deletion
    • The modal should close
    • You should see the "Authorized on" line disappear
craig-willis commented 4 years ago

Since @Xarthisius has a near complete PR for the API https://github.com/whole-tale/girder_wholetale/pull/364, can we get this working with the backend instead of placeholder text? This would also motivate testing his PR with the UI.

bodom0015 commented 4 years ago

Is it nearly complete? It is still marked [WIP] and only one of the three endpoints appears to work for my user.

In any case, I wasn't expecting as much feedback on the last PR, so I am going to address that feedback before going further with this one. I will add some review comments to @Xarthisius' PR about the errors that I'm hitting.

Xarthisius commented 4 years ago

Is it nearly complete? It is still marked [WIP] and only one of the three endpoints appears to work for my user.

That's what I thought. I marked it [WIP] because I'm pushing some changes still. However they should not affect functionality. What errors are you getting?

bodom0015 commented 4 years ago

See https://github.com/whole-tale/girder_wholetale/pull/364#issuecomment-545521760 for technical details - I see that you've updated your branch though, so let me retest.

bodom0015 commented 4 years ago

PR and test case have been updated to also include #561 as requested, since that was the ticket that actually needed wiring up to the API.

For future reference, it doesn't make much sense to split up the tickets if you're just going to explicitly request that all of the work be done immediately and all at once anyway. I did not see a need to put such a rush on this new feature, but perhaps there is some deadline of which I am unaware?

In any case, this is ready for review. Have a lovely weekend.

craig-willis commented 4 years ago

@bodom0015 -- clearly a miscommunication and/or misunderstanding on my part.

My question above was meant so suggest that for https://github.com/whole-tale/dashboard/issues/560 you could use the GET API instead of placeholder HTML text, not that you should also go ahead with https://github.com/whole-tale/dashboard/issues/561. When I wrote up https://github.com/whole-tale/dashboard/issues/560 bullet 3, I hoped that the backend would be far enough along to just handle the list.

Mea culpa.

craig-willis commented 4 years ago

Overall this looks great. During testing I've noticed one problem and one detail missing from the original issue #561.

First, during step 6 the modal state doesn't clear and I'm seeing an error in the console:

wholetale-32dab42f9a5846a1c2ecf2ced60d4b7c.js:622 Uncaught (in promise) TypeError: this.set is not a function
    at Object.clearConnectExtAcctModal (wholetale-32dab42f9a5846a1c2ecf2ced60d4b7c.js:622)
    at wholetale-32dab42f9a5846a1c2ecf2ced60d4b7c.js:623

If I select "Connect Account" button immediately after configuring an API key, the target and key are sometimes present in the modal. I assume it's related to the above error.

Second, the "Get from Zenodo" link replaces the current window which, while not specified in the original issue or mockup, I assume would be better as a new tab/window. This requires the user to configure the token and copy/paste back into the "Connect Account" window.

bodom0015 commented 4 years ago

First

Good catch - @Xarthisius brought this up yesterday as well and I had forgotten to revisit it

Second

Also good catch - I thought of this during the dev call yesterday and (again) forgot to revisit

Both should be fixed now :+1:

bodom0015 commented 4 years ago

There's a fairly annoying, somewhat complex bug that I'm still trying to track down. Given the complicated setup it is possible that a user would never hit this in practice, but I've been wrong before.

Steps to reproduce

  1. Navigate to "Browse", then perform a hard reload of the page
  2. Expand the user dropdown and choose "Settings"
  3. Add a Zenodo API key for either target, then add a Dataverse API key for any target, and finally add a Zenodo API key for the other target
  4. Revoke all API keys
  5. Click "Connect Account" again, then close the modal by clicking "Cancel"
    • Note that you should see the modal animation cut off abruptly as it is closing - this means that you are now in a bad state
  6. Navigate to "Browse", then back to "Settings"
  7. Click "Connect Account" one more time
    • Note that this modal does not offer any provider targets
  8. Close the modal by clicking "Cancel"

Expected Results

Actual Results

Xarthisius commented 4 years ago

@bodom0015 I've hit a variation of what you described with a simpler scenario:

  1. Navigate to "Browse", then perform a hard reload of the page
  2. Expand the user dropdown and choose "Settings"
  3. Open API key modal for Zenodo and dismiss it clicking on x in top-right corner.
  4. Navigate to "Browse".

Repating 2.-4. n times, will result in n modals.

bodom0015 commented 4 years ago

@Xarthisius thank you! Curious.. I will use this to try and track down what could be happening there :+1:

bodom0015 commented 4 years ago

Dug into this a bit, and it seems that anywhere we see this same behavior anywhere that don't use the {{#ui-modal}} helper for EmberJS.

The Select Data / Tale Workspaces modals use the correct helper, but the Publish modal does not. The issue here seems to be that navigations do not appear to be reset the DOM, and cause a duplicate modal to be inserted the next time it is requested. I've retested with the Publish modal and, while there is no duplicate shown, I did run into the state where the Cancel button did not function. Perhaps that could explain the strange behavior we see where the Cancel button suddenly stops functioning?

It seems like if we use an ID selector (e.g. $('#connect-apikey-modal')), instead of a class selector (e.g. $('.ui.modal.apikey')), I believe the default behavior is to stop after the first instance is selected (where a class selector will select all matching instances). This would hide the problem from the user, but unfortunately I don't think it is the correct solution to the problem.

At the time, my modals in Chrome's Element inspector looked as follows:

<div class="ui dimmer modals page transition hidden">
    <div id="connect-apikey-modal" class="ui modal apikey transition hidden">...</div>
    <div id="revoke-apikey-modal" class="ui modal revoke-apikey transition hidden">...</div>
    <div id="revoke-apikey-modal" class="ui modal revoke-apikey transition hidden">...</div>
    <div id="revoke-apikey-modal" class="ui modal revoke-apikey transition hidden">...</div>
    <div id="connect-apikey-modal" class="ui modal apikey transition hidden">...</div>
    <div id="publish-modal" class="ui publish modal transition visible active" style="display: block !important;">...</div>
    <div id="publish-modal" class="ui publish modal transition hidden">...</div>

Note that even in this case, as long as we use an ID selector only one modal should be visible and active at a time.

The right way to fix this is probably to use the {{#ui-modal}} helper everywhere, but retrofitting these modals and the publish-modal to follow that pattern may be slightly more involved than I was hoping it would be.

bodom0015 commented 4 years ago

Confirmed with the publish-modal - this same bug is the reason why the Cancel button sometimes stop working on that modal dialog. Will likely revisit this modal when adding the Zenodo publishing feature(s)

bodom0015 commented 4 years ago

Updated PR to include an attempt to cover-up the duplicate modal issue. Seems like it isn't visible to the user as long as we use an ID selector, although inspecting with the debugger shows us that the modals are still there (just hidden).

I also included a fix for the footer... Previously, content above the footer that was long enough would cause it to improperly float over and obscure the page content. I believe this should have been position: fixed all along, but perhaps I'm just not thinking clearly.

Finally, I've added support for provider.type=='bearer' and provider.type=='dataone' by redirecting to provider.url on Connect. To Disconnect, we call the /revoke endpoint just as before. Note that the Connect button must be disabled when provider.state=='authorized' or provider.state=='preauthorized', since this changes the provider.url to a different (invalid) value.

ThomasThelen commented 4 years ago

Here are two variants that have a similar workflow to the double-modal issue

1. Open the davaverse connection modal
2. Navigate to browse
3. Navigate to settings
4. Open the dataverse modal
5. Note that you can't select any repositories
6. Close the modal
7. Re-open it
8. Note that you can now make a selection
1. Open the dataverse connection modal
2. Navigate to browse
3. Navigate to settings
4. Open the dataverse modal
5. Navigate to browse
6. Navigate to Settings
7. Open the zenodo modal
8. Close it
9. Open the zenodo modal
10. See that you get the dataverse modal
bodom0015 commented 4 years ago

I have filed #572 to track the duplicate modal issue