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

Mccalluc/user files sorting #1950

Merged
merged 12 commits into from Aug 1, 2017
Merged

Mccalluc/user files sorting #1950

merged 12 commits into from Aug 1, 2017

Conversation

mccalluc
Copy link
Member

@mccalluc mccalluc commented Jul 31, 2017

There are still bugs in the sorting, but this is a start. (For example, other columns seem to work, but not "filename"?)

Changes to utils.py not required in this PR, but they are part of the general idea of failing fast and giving enough information to debug the problem.

Trying to reproduce the test failure locally...

@mccalluc mccalluc requested a review from scottx611x July 31, 2017 16:34
@codecov-io
Copy link

codecov-io commented Jul 31, 2017

Codecov Report

Merging #1950 into develop will decrease coverage by 0.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1950      +/-   ##
===========================================
- Coverage    42.78%   42.77%   -0.02%     
===========================================
  Files          414      415       +1     
  Lines        26988    27015      +27     
  Branches      1321     1322       +1     
===========================================
+ Hits         11548    11556       +8     
- Misses       15440    15459      +19
Impacted Files Coverage Δ
refinery/data_set_manager/utils.py 51.42% <0%> (-0.59%) ⬇️
...e/js/user-file-browser/services/user-file-sorts.js 100% <100%> (ø)
...file-browser/ctrls/user-file-browser-files-ctrl.js 13.79% <7.14%> (-11.21%) ⬇️
.../source/js/user-file-browser/services/user-file.js 75% <83.33%> (ø) ⬆️

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 e9f4100...d6f0259. Read the comment docs.

))
except KeyError:
raise RuntimeError(
'Not expected response structure: %s', response_obj
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.format()

response_obj = json.loads(full_response.content)
except ValueError:
raise RuntimeError(
'Expected JSON from SOLR, not: %s', full_response.content
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.format()

function UserFileBrowserFilesCtrl (
$log,
$q,
uiGridConstants,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need uiGridConstants anymore

@@ -160,7 +160,7 @@
.expectGET(
settings.appRoot +
settings.refineryApiV2 +
'/user/files/?fq=&limit=100'
'/user/files/?fq=&limit=100&sort='
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for appropriate structure after sort=

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, now that I look at it, that is already being done with fakeResponse.

@mccalluc
Copy link
Member Author

mccalluc commented Aug 1, 2017

@scottx611x : Comments addressed. Travis failure on the push build isn't something I've seen before, but I don't think it's related to anything in the PR.

Ok to merge?

@scottx611x
Copy link
Member

@mccalluc Hmm thats an odd one, looks like an Issue with that CI build itself. I've just restarted it, lets see it pass first just as a sanity check.

@mccalluc mccalluc merged commit 49ce577 into develop Aug 1, 2017
@mccalluc mccalluc deleted the mccalluc/user-files-sorting branch August 1, 2017 13:53
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