check if analysis is public or owned before rendering analysis builder in routes #764
Conversation
|
Would it be possible to go into "view" mode instead? I think people may want to share private stuff ala NeuroVault (if you know the adress you can access it). But maybe not edit it (since the API won't let you anyways). @tyarkoni what do you think? |
|
@rwblair this analysis Ma40P is not private and PASSED. Yet this PR's frontend is not letting me see it. Any ideas? |
|
@rwblair also worth noting that the analysis loads fine if the user navigates from the public dashboard; it just can't be entered directly into the location bar. |
|
Hmm putting this logic in the routes directly might not be the best place for it. No current guarantee of certain props or states having loaded yet. I'll think about it some more. |
|
@rwblair cool, thanks. Just to clarify, I meant that it currently works if you navigate to it, but not if you enter the url directly. The desired behavior is that it should be visible no matter how you got there, as long as you have the right URL. ((I think we're on the same page, but just wanted to make sure.) |
Codecov Report
@@ Coverage Diff @@
## master #764 +/- ##
==========================================
+ Coverage 83.16% 83.49% +0.32%
==========================================
Files 63 63
Lines 2834 2956 +122
==========================================
+ Hits 2357 2468 +111
- Misses 477 488 +11
Continue to review full report at Codecov.
|
|
Ok shifted this check into the builder constructor. Instead of redirecting front page it now modifies which analysis statuses are considered editable so the non owner is dropped directly into the review tab even if analysis is still a draft. This is wonky in the case that the analysis has say selected a dataset/task but no predictors. The plot generation fails and there isn't much information in the review section. Do we want text for a non owner of a draft dataset to give context of whats going on or what they're looking at? The builder currently only has a boolean if the user owns the analysis or not, we could start tracking which user owns which analysis on the front end and have the builder display that information to non owners viewing an analysis. |
|
Oh, I don't think analyses should be viewable by anyone other than the owner unless they've actually been locked and executed. If you mean, what happens if there's an analysis that's technically been completed, but doesn't have any predictors, then it's fine if the review/plotting stuff fails; that's the owner's problem. But I don't think other people should be able to see analyses that are still open for editing, whether or not they've selected predictors. (Sorry, should have clarified that earlier.) I do think the front end should probably display ownership information... though doing that right would probably require adding profile pages that list each user's public analyses etc. Worth opening an issue for, but not a high priority (although I'm lagging on the Neurosynth stuff I promised to get you, so I suppose if you have some cycles, adding this makes sense). |
|
Wait, I thought that non owners should be able to see any analysis they
have the URL for, including drafts? (Just not be able to edit them)
I think this would be useful if you want to share a report for an analysis
that is a WIP and not compiled yet.
Also don't feel that strongly though.
There's a really old issue got adding profiles.
…On Thu, May 21, 2020, 9:40 AM Tal Yarkoni ***@***.***> wrote:
Oh, I don't think analyses should be viewable by anyone other than the
owner unless they've actually been locked and executed. If you mean, what
happens if there's an analysis that's technically been completed, but
doesn't have any predictors, then it's fine if the review/plotting stuff
fails; that's the owner's problem. But I don't think other people should be
able to see analyses that are still open for editing, whether or not
they've selected predictors. (Sorry, should have clarified that earlier.)
I do think the front end should probably display ownership information...
though doing that right would probably require adding profile pages that
list each user's public analyses etc. Worth opening an issue for, but not a
high priority (although I'm lagging on the Neurosynth stuff I promised to
get you, so I suppose if you have some cycles, adding this makes sense).
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#764 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAVFLMFZVUET44O2CR53RDTRSU4MFANCNFSM4NEEMTRQ>
.
|
|
I guess viewing drafts would be okay as long as it's clear they haven't been finalized... But if we want to allow that then I do think it needs to be made clear in some way who owns the analysis, and that it's not yet locked (e.g., with a red warning bar at the top saying something like "This analysis is still in the editing process and has not yet been finalized") |
|
@adelavega current state of this pr will let any one view a draft if they have the url. Can you look at the alert shown to non owners and let me know what it should look like and what it should say, its very basic at the moment. |
|
Hey @rwblair I think that works for now. Maybe it can be more explicit: "This analysis draft is currently read only. Only the owner of this draft can edit" |
|
The only minor niggle is a few pixels of spacing below the warning would be ideal. Otherwise LGTM. We can circle back to this once we implement profiles. |
|
Ok updated the text and added a .5 em bottom margin. |
Fixes #759.
Redirects back to main page and displays a warning if user attempts to browse non public dataset that they don't own.
This would prevent private analyses from being viewed by non owners who have the id, is that something we want?