Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Link to neurovault uploads. #584

Merged
merged 4 commits into from Jun 6, 2019

Conversation

rwblair
Copy link
Collaborator

@rwblair rwblair commented Jun 2, 2019

Currently will only display latest failed attempt with traceback, and will only show latest pending attempt with traceback if there are no successes. All success shown if available. Not sure how to word the failed and pending titles.

API endpoint currently only hit on initial analysis load and when the analysisId changes.

@codecov-io
Copy link

codecov-io commented Jun 2, 2019

Codecov Report

Merging #584 into master will decrease coverage by 4.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage    76.5%   72.38%   -4.13%     
==========================================
  Files          62       53       -9     
  Lines        2733     2053     -680     
==========================================
- Hits         2091     1486     -605     
+ Misses        642      567      -75
Impacted Files Coverage Δ
neuroscout/tests/request_utils.py
neuroscout/tests/api/test_dataset.py
neuroscout/tests/api/test_user.py
neuroscout/tests/api/test_predictor.py
neuroscout/tests/test_views.py
neuroscout/tests/api/test_analyses.py
neuroscout/tests/test_models.py
neuroscout/tests/conftest.py
neuroscout/tests/api/test_run_task.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fefcb49...f123612. Read the comment docs.

@adelavega
Copy link
Collaborator

Nice! Thanks! This looks great. I may push a change with minor aesthetic tweaks but otherwise I think this is what we need.

Question: what do you mean by:

API endpoint currently only hit on initial analysis load and when the analysisId changes.

What if you're looking at Status and then an upload gets pushed. Maybe we can have a little "refresh" icon? Or I guess it's probably reasonable for people to expect to have to refresh the page to get the latest?

@adelavega
Copy link
Collaborator

Minor (more of a note to self): Let's format this timestamp nicer Uploaded at: 2019-05-27T19:03:00

@adelavega
Copy link
Collaborator

If there is nothing to show, either have a place holder below "NeuroVault Uploads", or don't show. The latter is probably best, especially if it hasn't been compiled yet. Maybe after compilation it can have a place holder

@rwblair
Copy link
Collaborator Author

rwblair commented Jun 4, 2019

Unfortunately Date.parse in the app wasn't able to parse '2019-05-27T19:03:00' (though my local node install was). Need to find another way of parsing (or can start just replace that inner T with a space? or remove the time part all together?)

@adelavega
Copy link
Collaborator

adelavega commented Jun 5, 2019 via email

@adelavega
Copy link
Collaborator

Is this still WIP? Looks pretty good to me.

@adelavega
Copy link
Collaborator

I'm going to merge, it looks good to me

@adelavega adelavega merged commit d2ae958 into neuroscout:master Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants