8941 adding file count in solr (v2)#10598
Conversation
pdurbin
left a comment
pdurbin
left a comment
There was a problem hiding this comment.
@luddaniel overall, this looks great. Nice feature. I didn't run the code but it makes sense from a quick look.
I did leave some comments.
@GPortas you might want to review at this PR because it touches code you add in this PR:
| indexService.datasetVersionFilesServiceBean = Mockito.mock(DatasetVersionFilesServiceBean.class); | ||
| BrandingUtil.injectServices(indexService.dataverseService, indexService.settingsService); | ||
|
|
||
| Mockito.when(indexService.dataverseService.findRootDataverse()).thenReturn(dataverse); |
There was a problem hiding this comment.
Is it worth adding a test to SearchIT.java?
There was a problem hiding this comment.
-
As IT is to test output of API call, I would say no nothing change here and there is already "something" https://github.com/IQSS/dataverse/blob/develop/src/test/java/edu/harvard/iq/dataverse/api/SearchIT.java#L433
-
I Tried to add a test inside
IndexServiceBeanTest.javabut as we play with mocked dataset, datafile, metadatafile and mocked DatasetVersionFilesServiceBean I ended up withfileCount=0So I don't see a good way to add a test.
| } | ||
| if (!JvmSettings.UI_SHOW_VALIDITY_LABEL_WHEN_PUBLISHED.lookupOptional(Boolean.class).orElse(true)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Are these tabs? Can they please be reverted to spaces?
There was a problem hiding this comment.
My IntelliJ IDEA says it is 4 spaces tab spacing. Anyway, I fixed this because indentation was broken inside public boolean isValid(Predicate<SolrSearchResult> canUpdateDataset) {
|
@pdurbin it will be a forseken PR soon ;) |
|
@luddaniel hmm? Sorry, what do you mean? 😅 |
|
@pdurbin this PR has been updated with |
|
@pdurbin Is it in |
|
@cmbz @scolapasta what do you think? |
stevenwinship
left a comment
stevenwinship
left a comment
There was a problem hiding this comment.
Looks good to me. I don't see any negative comments from anyone about this change so I will be approving.
|
No issues observed during testing. Merging PR Screen.Recording.2024-11-22.at.3.24.47.PM.movScreen.Recording.2024-11-22.at.3.20.41.PM.mov |
|
I just asked @donsizemore to reindex the beta server (thanks!) and now we can see this feature working. I went to https://beta.dataverse.org/dataverse/root/?q=fileCount:25 and found a dataset with 25 files (cat pictures, it seems)
|
7 participants
This PR is following the closed PR #9823 with a branch up to date and more details.
What this PR does / why we need it:
This PR handles 2 subjects as one go :
http://localhost:8080/api/search?q=fileCount:0)&per_page=1000)two birds one stone
Which issue(s) this PR closes:
Special notes for your reviewer:
As the original PR was not merged after 9 month in open state, some competing developments were made on the same lines of code. I just finished the rework after merging develop a0028c3 and noticed that @GPortas was related to those developments. @GPortas can you please be part of reviewers and approve this PR ?
Suggestions on how to test this:
Checking Search API results of dataset version with files and no file.
Is there a release notes update needed for this change?:
May be not required, tell me.
Demo:
filecount_demo.mp4
Screenshot of API Search result of the same dataset after publishing