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

Add parasolr last modified mixin (#375) #669

Merged
merged 9 commits into from
Mar 2, 2022

Conversation

kmcelwee
Copy link
Contributor

@kmcelwee kmcelwee requested a review from rlskoeser February 28, 2022 19:58
Copy link
Contributor

@rlskoeser rlskoeser left a comment

Choose a reason for hiding this comment

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

For the search I think we do want all documents — we only include public/non-suppressed documents in the search, but if a document was marked as suppressed that should be reflected in the search last-modified.

For the document detail views, we want the last modification for the single document being viewed. Please apply to all document detail views — perhaps generalize/rename DocumentPastIdMixin to a document detail base class and add the last-modified behavior there.

@rlskoeser
Copy link
Contributor

* Looks like next time we do a release for parasolr we should make sure to rebuild the docs. https://parasolr.readthedocs.io/en/latest/search.html?q=SolrLastModifiedMixin&check_keywords=yes&area=default

I'm not sure the view was added to the code docs, please check. (I didn't even remember the view had been generalized and moved to parasolr)

@kmcelwee
Copy link
Contributor Author

kmcelwee commented Mar 2, 2022

@rlskoeser

For the search I think we do want all documents — we only include public/non-suppressed documents in the search, but if a document was marked as suppressed that should be reflected in the search last-modified.

I tried testing this, and it seems to work as is. Would you mind confirming that this is testing the behavior you're looking for? 2bd1b05

@rlskoeser
Copy link
Contributor

@rlskoeser

For the search I think we do want all documents — we only include public/non-suppressed documents in the search, but if a document was marked as suppressed that should be reflected in the search last-modified.

I tried testing this, and it seems to work as is. Would you mind confirming that this is testing the behavior you're looking for? 2bd1b05

Yes, the existing logic is correct; this is a good test to confirm it.

@kmcelwee
Copy link
Contributor Author

kmcelwee commented Mar 2, 2022

I'm not sure the view was added to the code docs, please check. (I didn't even remember the view had been generalized and moved to parasolr)

It wasn't. I made a PR: Princeton-CDH/parasolr#71

@kmcelwee
Copy link
Contributor Author

kmcelwee commented Mar 2, 2022

For the document detail views, we want the last modification for the single document being viewed. Please apply to all document detail views — perhaps generalize/rename DocumentPastIdMixin to a document detail base class and add the last-modified behavior there.

c5c3a0f

@kmcelwee kmcelwee requested a review from rlskoeser March 2, 2022 18:32
geniza/corpus/views.py Show resolved Hide resolved
geniza/corpus/views.py Outdated Show resolved Hide resolved
geniza/corpus/tests/test_corpus_views.py Outdated Show resolved Hide resolved
geniza/corpus/tests/test_corpus_views.py Outdated Show resolved Hide resolved
geniza/corpus/tests/test_corpus_views.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 2, 2022

Codecov Report

Merging #669 (7e57d3f) into develop (b0ff08f) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #669      +/-   ##
===========================================
+ Coverage    98.64%   98.74%   +0.10%     
===========================================
  Files          121      121              
  Lines         5677     6159     +482     
===========================================
+ Hits          5600     6082     +482     
  Misses          77       77              

@kmcelwee kmcelwee requested a review from rlskoeser March 2, 2022 19:19
"""
return {"pgpid_i": self.kwargs["pk"]}
"""Filter solr last modified query by pgpid"""
return {"pgpid_i": self.kwargs["pk"], "item_type_s": "document"}
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry if I was unclear: we need the item type filter on the search view. (I wasn't thinking about adding it here: it may not strictly be necessary here, but it's probably a good idea)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh whoops, yeah, here it is 7e57d3f

@kmcelwee kmcelwee requested a review from rlskoeser March 2, 2022 21:16
@kmcelwee kmcelwee merged commit 5e52f54 into develop Mar 2, 2022
@kmcelwee kmcelwee deleted the 375-add-parasolr-last-modified-mixin branch March 2, 2022 21:25
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.

3 participants