Fix: publicationTitle filter only works for projects (#2764) #2792

amarjandu · 2021-02-12T19:53:12Z

PR title references issue
Title of main commit references issue
PR is linked to Zenhub issue

Author (reindex)

Added r tag to commit title _{or this PR does not require reindexing}
Added reindex label to PR _{or this PR does not require reindexing}

Author (freebies & chains)

Freebies are blocked on this PR _{or there are no freebies in this PR}
Freebies are referenced in commit titles _{or there are no freebies in this PR}
This PR is blocked by previous PR in the chain _{or this PR is not chained to another PR}
Added chain label to the blocking PR _{or this PR is not chained to another PR}

Author (upgrading)

Documented upgrading of deployments in UPGRADING.rst _{or this PR does not require upgrading}
Added u tag to commit title _{or this PR does not require upgrading}
Added upgrade label to PR _{or this PR does not require upgrading}

Author (requirements, before every review)

Ran make requirements_update _{or this PR leaves requirements*.txt, common.mk and Makefile untouched}
Added R tag to commit title _{or this PR leaves requirements*.txt untouched}
Added reqs label to PR _{or this PR leaves requirements*.txt untouched}

Author (before every review)

make integration_test passes in personal deployment _{or this PR does not touch functionality that could break the IT}
Rebased branch on develop, squashed old fixups

Primary reviewer (after approval)

Commented on demo expectations _{or labelled PR as no demo}
Decided if PR can be labeled no sandbox
PR title is appropriate as title of merge commit
Moved ticket to Approved column
Assigned PR to an operator

Operator (before pushing merge the commit)

Operator (after pushing the merge commit)

Moved freebies to Merged column _{or there are no freebies in this PR}
Shortened the PR chain _{or this PR is not the base of another PR}
Verified that N reviews labelling is accurate
Pushed merge commit to Gitlab _{or this changes can be pushed later, together with another PR}
Deleted PR branch from Github and Gitlab

Operator (reindex)

Purchased BQ slot committment in dev _{or this PR does not require reindexing dev}
Started reindex in dev _{or this PR does not require reindexing dev}
Deleted BQ slot committment in dev _{or this PR does not require reindexing dev}
Checked for failures in dev _{or this PR does not require reindexing dev}
Purchased BQ slot committment in prod _{or this PR does not require reindexing prod}
Started reindex in prod _{or this PR does not require reindexing prod}
Deleted BQ slot committment in prod _{or this PR does not require reindexing prod}
Checked for failures in prod _{or this PR does not require reindexing prod}

Operator

Unassigned PR

codecov · 2021-02-12T20:15:15Z

Codecov Report

Merging #2792 (f696bb0) into develop (905b700) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2792      +/-   ##
===========================================
+ Coverage    83.42%   83.43%   +0.01%     
===========================================
  Files          127      127              
  Lines        13279    13291      +12     
===========================================
+ Hits         11078    11090      +12     
  Misses        2201     2201

Impacted Files	Coverage Δ
src/azul/plugins/metadata/hca/aggregate.py	`97.70% <ø> (ø)`
test/service/test_response.py	`99.81% <100.00%> (+<0.01%)`	⬆️

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 905b700...f696bb0. Read the comment docs.

coveralls · 2021-02-12T20:15:35Z

Coverage increased (+0.01%) to 83.393% when pulling f696bb0 on issues/amar/2764-publicationTitles into 905b700 on develop.

hannes-ucsc · 2021-02-13T00:52:22Z

Title makes no sense, @amarjandu. The publication titles ARE present in the project aggregate. Where they are missing from is the OTHER aggregates.

dsotirho-ucsc

Just one typo to clean up.

Verified fix by comparing dev (0 hits) against personal deployment (40 hits). https://service.dev.singlecell.gi.ucsc.edu/index/files?filters=%7B%22publicationTitle%22%3A%7B%22is%22%3A%5B%22A%20single-cell%20molecular%20map%20of%20mouse%20gastrulation%20and%20early%20organogenesis%22%5D%7D%7D&size=50&sort=projectTitle&order=asc&catalog=dcp2

test/indexer/data/aaa96233-bf27-44c7-82df-b4dc15ad4d9d.2018-11-02T113344.698028Z.results.json

dsotirho-ucsc

Approved.
Travis reports an error however it is due to codecov failing and does not appear to be due to the PR itself.

amarjandu · 2021-02-18T18:12:34Z

Travis reports an error however it is due to codecov failing and does not appear to be due to the PR itself.

Thanks for the heads up, pipeline restarted.

hannes-ucsc

There should be a test, likely in TestResponse, that shows that we can filter a non-project index by publication title.

hannes-ucsc

I think we've been over this a couple times. There's an affinity to overly verbose variable names. More verbose isn't always better. It's worse, in many cases, because it makes the code more noisy, more likely to be in need of wrapping.

Whenever you have test + fix you should do a split commit. I think the C guide stipulates this. Could you check, please?

hannes-ucsc · 2021-02-24T22:48:07Z

test/service/test_response.py

@@ -1772,6 +1772,61 @@ def test_pagination_search_after_search_before(self):
        self.assertEqual(second_page_previous['search_before'], 'null')
        self.assertEqual(second_page_previous['search_before_uid'], 'doc#308eea51-d14b-4036-8cd1-cfd81d7532c3')

+    def test_filter_by_publicationTitle(self):


Suggested change

def test_filter_by_publicationTitle(self):

def test_filter_by_publication_title(self):

just a heads up there is precedent in this TestClass to have the facet name within the test definition. test_filter_by_projectId was the one that I saw.

hannes-ucsc · 2021-02-24T22:49:20Z

test/service/test_response.py

@@ -1772,6 +1772,61 @@ def test_pagination_search_after_search_before(self):
        self.assertEqual(second_page_previous['search_before'], 'null')
        self.assertEqual(second_page_previous['search_before_uid'], 'doc#308eea51-d14b-4036-8cd1-cfd81d7532c3')

+    def test_filter_by_publicationTitle(self):
+        expected_filtered_results = [


Suggested change

expected_filtered_results = [

cases = [

small scope, short variable name

hannes-ucsc · 2021-02-24T22:51:22Z

test/service/test_response.py

+        ]
+        for publication_title, bundle_hits in expected_filtered_results:
+            with self.subTest(publication_title=publication_title):
+                url = furl(url=self.base_url, path='index/bundles').url


The ticket doesn't use bundles. It uses files I think. Ideally, the test resembles the repro in the ticket.

hannes-ucsc · 2021-02-24T22:51:59Z

test/service/test_response.py

+                ()
+            )
+        ]
+        for publication_title, bundle_hits in expected_filtered_results:


Suggested change

for publication_title, bundle_hits in expected_filtered_results:

for title, bundles in expected_filtered_results:

hannes-ucsc · 2021-02-24T22:52:21Z

test/service/test_response.py

+        for publication_title, bundle_hits in expected_filtered_results:
+            with self.subTest(publication_title=publication_title):
+                url = furl(url=self.base_url, path='index/bundles').url
+                filters_parameter = {


Suggested change

filters_parameter = {

filters = {

hannes-ucsc · 2021-02-24T22:53:35Z

test/service/test_response.py

+                        'is': [publication_title]
+                    }
+                }
+                publication_term_facet = {


Suggested change

publication_term_facet = {

expected_terms = {

test/service/test_response.py

hannes-ucsc

Missed from previous review:

Whenever you have test + fix you should do a split commit. I think the C guide stipulates this. Could you check, please?

hannes-ucsc · 2021-02-26T20:59:30Z

test/service/test_response.py

+                self.assertEqual(expected_terms,
+                                 response.json()['termFacets']['publicationTitle'])
+                actual_files = {
+                    one(hit['files'])['uuid'] for hit in response.json()['hits']


https://github.com/DataBiosphere/azul/blob/develop/CONTRIBUTING.rst#14line-wrapping-and-indentation

hannes-ucsc · 2021-02-26T21:05:45Z

And https://github.com/DataBiosphere/azul/blob/develop/CONTRIBUTING.rst#511review-comments

there are some open conversations that need 👍

amarjandu · 2021-03-03T01:39:01Z

@hannes-ucsc

Whenever you have test + fix you should do a split commit. I think the C guide stipulates this. Could you check, please?

My apologies, I saw this, then missed addressing it. I'll add Double checking the top-level review comment to my checklist of things to do before requesting review, to avoid this happening again.

I was NOT able to locate the section that you mentioned above, I read through all sections that mentioned test and fix as well as the Split commits section specifically.
Perhaps we can add it to the Split commits section where there is a mention about the body being used to distinguish parts of a split commit.

  - ``[1/2] Reticulate them splines for good measure (#123)`` and
        Add tests for spline counting
  - ``[2/2] Reticulate them splines for good measure (#123)``
        Reticulate splines before execution

In the Testing section it might be worthwhile to add the expectation of having a commit that causes test failures, then a commit that fixes the failures (and the issue).

there are some open conversations that need 👍

I believe this has been done.

hannes-ucsc · 2021-03-08T18:55:10Z

test/service/test_response.py

+                actual_files = {
+                    one(hit['files'])['uuid']
+                    for hit
+                    in response.json()['hits']


No wrap before in.
See examples in
https://github.com/DataBiosphere/azul/blob/develop/CONTRIBUTING.rst#14line-wrapping-and-indentation

hannes-ucsc · 2021-03-08T18:58:29Z

test/service/test_response.py

+                set()
+            )
+        ]
+        for title, files in cases:


Suggested change

for title, files in cases:

for title, expected_files in cases:

hannes-ucsc · 2021-03-08T18:58:57Z

test/service/test_response.py

+                self.assertEqual(200, response.status_code)
+                self.assertEqual(expected_terms,
+                                 response.json()['termFacets']['publicationTitle'])
+                actual_files = {


Suggested change

actual_files = {

files = {

Should be consistent with prefixing either actual or expected but not mixing both styles.

hannes-ucsc · 2021-03-08T23:52:07Z

For demo, attempt to reproduce the issue.

Add tests to filter by publication_title

amarjandu · 2021-03-11T16:29:15Z

5 failed notifications found the failed notification queue, they are already noted in:
#2870
#2873
Output:
azul-notifications_fail-dev.json.txt

github-actions bot added the orange [process] Done by the Azul team label Feb 12, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch 2 times, most recently from 52fc6a1 to c19ecd4 Compare February 17, 2021 00:43

amarjandu changed the title ~~Include publication_titles within project aggregates (#2764)~~ Accumulate publication_titles for aggregations (#2764) Feb 17, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch from c19ecd4 to 2e11d9b Compare February 17, 2021 00:44

amarjandu assigned dsotirho-ucsc Feb 17, 2021

dsotirho-ucsc requested changes Feb 17, 2021

View reviewed changes

test/indexer/data/aaa96233-bf27-44c7-82df-b4dc15ad4d9d.2018-11-02T113344.698028Z.results.json Outdated Show resolved Hide resolved

dsotirho-ucsc removed their assignment Feb 17, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch from a871442 to 600938d Compare February 17, 2021 22:59

amarjandu assigned dsotirho-ucsc Feb 17, 2021

dsotirho-ucsc approved these changes Feb 18, 2021

View reviewed changes

dsotirho-ucsc removed their assignment Feb 18, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch 2 times, most recently from ced87fb to 7660e59 Compare February 18, 2021 19:32

amarjandu changed the title ~~Accumulate publication_titles for aggregations (#2764)~~ Fix: publicationTitle filter only works for projects (#2764) Feb 18, 2021

amarjandu assigned hannes-ucsc Feb 19, 2021

hannes-ucsc requested changes Feb 23, 2021

View reviewed changes

hannes-ucsc removed their assignment Feb 23, 2021

hannes-ucsc added the 1 review [process] Lead requested changes once label Feb 23, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch 3 times, most recently from 5e3a2c3 to 35bda73 Compare February 24, 2021 18:06

amarjandu assigned hannes-ucsc Feb 24, 2021

hannes-ucsc requested changes Feb 24, 2021

View reviewed changes

hannes-ucsc removed their assignment Feb 24, 2021

hannes-ucsc removed the 1 review [process] Lead requested changes once label Feb 24, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch from 3dd3748 to 69b4adf Compare February 26, 2021 17:39

amarjandu assigned amarjandu and hannes-ucsc and unassigned amarjandu Feb 26, 2021

hannes-ucsc requested changes Feb 26, 2021

View reviewed changes

hannes-ucsc added 4+ reviews [process] Lead requested changes four times or more and removed 3 reviews [process] Lead requested changes thrice labels Feb 26, 2021

hannes-ucsc removed their assignment Feb 26, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch from 69b4adf to 78290cd Compare March 3, 2021 01:46

amarjandu assigned hannes-ucsc Mar 3, 2021

hannes-ucsc requested changes Mar 8, 2021

View reviewed changes

hannes-ucsc removed their assignment Mar 8, 2021

amarjandu force-pushed the issues/amar/2764-publicationTitles branch from 4938a2a to e906d2b Compare March 8, 2021 21:16

amarjandu assigned hannes-ucsc Mar 8, 2021

hannes-ucsc approved these changes Mar 8, 2021

View reviewed changes

hannes-ucsc assigned jessebrennan and unassigned hannes-ucsc Mar 8, 2021

jessebrennan assigned amarjandu and unassigned jessebrennan Mar 9, 2021

amarjandu added 2 commits March 10, 2021 12:09

[r 1/2] Accumulate publication_titles for aggregations (#2764)

1eff06c

Add tests to filter by publication_title

[r 2/2] Accumulate publication_titles for aggregations (#2764)

f696bb0

amarjandu force-pushed the issues/amar/2764-publicationTitles branch from e906d2b to f696bb0 Compare March 10, 2021 20:12

amarjandu added the reindex:dev [process] PR requires reindexing dev label Mar 10, 2021

amarjandu merged commit 8f15f20 into develop Mar 10, 2021

amarjandu deleted the issues/amar/2764-publicationTitles branch March 10, 2021 22:52

amarjandu removed their assignment Mar 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: publicationTitle filter only works for projects (#2764) #2792

Fix: publicationTitle filter only works for projects (#2764) #2792

amarjandu commented Feb 12, 2021 •

edited

Loading

codecov bot commented Feb 12, 2021 •

edited

Loading

coveralls commented Feb 12, 2021 •

edited

Loading

hannes-ucsc commented Feb 13, 2021 •

edited

Loading

dsotirho-ucsc left a comment

dsotirho-ucsc left a comment

amarjandu commented Feb 18, 2021

hannes-ucsc left a comment

hannes-ucsc left a comment

hannes-ucsc Feb 24, 2021

amarjandu Feb 24, 2021

hannes-ucsc Feb 24, 2021

hannes-ucsc Feb 24, 2021

hannes-ucsc Feb 24, 2021

hannes-ucsc Feb 24, 2021

hannes-ucsc Feb 24, 2021

hannes-ucsc left a comment

hannes-ucsc Feb 26, 2021

hannes-ucsc commented Feb 26, 2021

amarjandu commented Mar 3, 2021

hannes-ucsc Mar 8, 2021

hannes-ucsc Mar 8, 2021

hannes-ucsc Mar 8, 2021

hannes-ucsc commented Mar 8, 2021

amarjandu commented Mar 11, 2021

	def test_filter_by_publicationTitle(self):
	def test_filter_by_publication_title(self):

	for publication_title, bundle_hits in expected_filtered_results:
	for title, bundles in expected_filtered_results:

	for title, files in cases:
	for title, expected_files in cases:

Fix: publicationTitle filter only works for projects (#2764) #2792

Fix: publicationTitle filter only works for projects (#2764) #2792

Conversation

amarjandu commented Feb 12, 2021 • edited Loading

codecov bot commented Feb 12, 2021 • edited Loading

Codecov Report

coveralls commented Feb 12, 2021 • edited Loading

hannes-ucsc commented Feb 13, 2021 • edited Loading

dsotirho-ucsc left a comment

Choose a reason for hiding this comment

dsotirho-ucsc left a comment

Choose a reason for hiding this comment

amarjandu commented Feb 18, 2021

hannes-ucsc left a comment

Choose a reason for hiding this comment

hannes-ucsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannes-ucsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannes-ucsc commented Feb 26, 2021

amarjandu commented Mar 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hannes-ucsc commented Mar 8, 2021

amarjandu commented Mar 11, 2021

amarjandu commented Feb 12, 2021 •

edited

Loading

codecov bot commented Feb 12, 2021 •

edited

Loading

coveralls commented Feb 12, 2021 •

edited

Loading

hannes-ucsc commented Feb 13, 2021 •

edited

Loading