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

Fixes file count in dashboard AdminSets #3154

Merged
merged 1 commit into from Sep 12, 2018
Merged

Fixes file count in dashboard AdminSets #3154

merged 1 commit into from Sep 12, 2018

Conversation

kdid
Copy link
Contributor

@kdid kdid commented Jul 17, 2018

Fixes #446

  • Show correct count of files in each AdminSet in Admin Dashboard

@samvera/hyrax-code-reviewers

@kdid kdid added this to Review in Hyrax WG -- Sprint 1 via automation Jul 18, 2018
@chrisdaaz chrisdaaz added this to Review in Hyrax WG -- Sprint 2 via automation Jul 30, 2018
@cjcolvar
Copy link
Member

cjcolvar commented Aug 1, 2018

@kdid The issue being fixed here mentions a limit of 10 files being reported. Could you please add a test that shows a count > 10?

@kdid
Copy link
Contributor Author

kdid commented Aug 2, 2018

@cjcolvar - I made an adjustment. Will this do it?

@laritakr laritakr requested a review from cjcolvar August 14, 2018 18:52
@laritakr
Copy link
Contributor

@kdid I'm having difficulty seeing how the spec actually tests the cause of the bug you are fixing to verify the fix. It appears to me that this is mocking out the search to provide results, but the search itself was the problem. I want to make sure that the specs are actually doing a meaningful test. Can you clarify?

@laritakr laritakr removed the request for review from cjcolvar August 15, 2018 17:05
@kdid
Copy link
Contributor Author

kdid commented Aug 15, 2018

@laritakr - What do you recommend?

The initial problem was that the code did not do an actual search for files in each AdminSet, but instead just counted whatever files happened to be contained in the first 10 rows of the search result that was designed to get the work count. (which is why it generally was 10 files but could have been more or less than 10 files depending on how many happened to be attached to those 10 works).

So now the .count_files method is doing a separate query to actually find the number of files in admin sets. This is a private method - I think we generally consider private methods implementation details and don't test them directly. The method under test: search_results_with_work_count is able to use the results from what's returned by the count_files method - whether it's below or above 10. I'm a little hesitant with our concerns with test performance to create 10+ actual objects.

@laritakr
Copy link
Contributor

@kdid

My approach would be to try to create solr docs only for the searches, and not the actual active fedora objects. I assume that only solr is needed for the searches. To be useful, the admin set service specs should verify that it can actually perform a search. From what I see, it is not testing returning an actual count of works or files, but it DOES query for admin sets.

If the admin set service is adequately tested, anywhere else in the code should be able to safely mock out the search results or create the bare minimum objects, and not have to test the search service itself. (You definitely don't want to test 10+ files in the feature spec where you are actually creating the objects).

@laritakr
Copy link
Contributor

@kdid you might want to look at how the collection_size_service_spec is setting up the solr docs for the specs and mimic that.

@no-reply no-reply added this to Review in Hyrax WG -- Sprint 3 Sep 4, 2018
@no-reply
Copy link
Member

@laritakr @cjcolvar does this latest commit address your test concerns?

@laritakr laritakr merged commit 0bbecda into master Sep 12, 2018
Hyrax WG -- Sprint 3 automation moved this from Review to Done Sep 12, 2018
@laritakr laritakr deleted the 446-file-count branch September 12, 2018 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants