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

feat(citations): add UnmatchedCitation model and logic #4949

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

grossir
Copy link
Contributor

@grossir grossir commented Jan 21, 2025

Solves #4920

  • Add new model UnmatchedCitation on citations app
  • refactor cl.search.models.Citation to create a BaseCitation abstract model to reuse on the UnmatchedCitation model
  • updates cl.citations.tasks.store_opinion_citations_and_update_parentheticals to handle storing and updating unmatched citations
  • updates cl.search.signals to update UnmatchedCitation status when a new Citation is saved
  • updates citations command find_citations to delete all UnmatchedCitation when the --all argument is passed
  • add tests
  • add update_unmatched_citations command to trigger update for found citations

@grossir grossir self-assigned this Jan 21, 2025
@grossir grossir force-pushed the 4920-store-unmatched-citations branch 2 times, most recently from 78468a9 to f5b3f35 Compare January 21, 2025 13:45
@grossir grossir marked this pull request as ready for review January 21, 2025 13:46
@grossir grossir force-pushed the 4920-store-unmatched-citations branch from f5b3f35 to e7eac4e Compare January 21, 2025 15:39
Solves #4920

- Add new model UnmatchedCitation on citations app
- refactor cl.search.models.Citation to create a BaseCitation abstract model to reuse on the UnmatchedCitation model
- updates cl.citations.tasks.store_opinion_citations_and_update_parentheticals to handle storing and updating unmatched citations
- updates cl.search.signals to update UnmatchedCitation status when a new Citation is saved
- add tests
- add update_unmatched_citations command to trigger update for found citations
@grossir grossir force-pushed the 4920-store-unmatched-citations branch from e7eac4e to e363c90 Compare January 21, 2025 16:20
@grossir
Copy link
Contributor Author

grossir commented Jan 21, 2025

@flooie this is ready for review. Can you review it or assign someone else? Thanks

@grossir grossir assigned flooie and unassigned grossir Jan 21, 2025
Copy link
Member

@mlissner mlissner left a comment

Choose a reason for hiding this comment

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

I didn't read all the code, but I did a quick look through at the models (since I always have opinions about them!). One or two little comments, but I'll leave the rest to others.

This seems well thought out and like a really nice improvement. It's going to be powerful to know which of our citations are the most needed.

cl/citations/models.py Outdated Show resolved Hide resolved
cl/citations/models.py Outdated Show resolved Hide resolved
grossir and others added 3 commits January 23, 2025 17:51
- update model and migrations files
- update status description and creation, given that an UnmatchedCitation may actually exist on the Citation table, but we are not able to resolve it
@flooie flooie assigned quevon24 and unassigned flooie Jan 24, 2025
@flooie
Copy link
Contributor

flooie commented Jan 24, 2025

@quevon24 can you review this please?

Copy link
Member

@quevon24 quevon24 left a comment

Choose a reason for hiding this comment

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

All looks good, only a suggestion related to the tests

cls.opinion = cls.cluster.sub_opinions.first()
UnmatchedCitation.objects.all().delete()

def test_1st_creation(self) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe split this big test into smaller tests, something like: test_save_unmatched_citations, test_signal_matching_citation and test_single_citation_resolved

Copy link
Contributor Author

@grossir grossir Jan 25, 2025

Choose a reason for hiding this comment

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

Originally I did it that way; but then I was inheriting from SimpleTestCase and it would break other tests (not precisely sure why).
Then I changed to TransactionTestCase as parent class, but the signal wouldn't work unless they were all in the same function. That's why I ended up putting everything in a single test
You can check the changes in this commit
4304ccb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: PRs to Review
Development

Successfully merging this pull request may close these issues.

4 participants