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

Topic landing pools sql #2110

Open
wants to merge 30 commits into
base: topic-landing-pools
Choose a base branch
from

Conversation

nsantacruz
Copy link
Contributor

@nsantacruz nsantacruz commented Nov 12, 2024

Description

Introduces the TopicPoolLink model, the first time we're using Django models in the system.

  • TopicPoolLink Model: Stores relationships between topics and pools, with pool and topic_slug fields.
  • Utility Methods for TopicPoolLink: Added methods to retrieve random topic slugs by pool and fetch pools by topic slug.
  • Enum-Based Pool Management: Introduced PoolType Enum for defining pool types (TEXTUAL, SHEETS, PROMOTED).
  • Settings Update: Registered admin_tools in INSTALLED_APPS.

Also refactors get_random_topic and related functions to utilize TopicPoolLink, as a proof-of-concept for TopicPoolLink IRL.

@@ -0,0 +1,99 @@
from django.contrib import admin, messages
from django_topics.models import Topic, TopicPool
from django_topics.models.pool import PoolType
Copy link
Contributor

Choose a reason for hiding this comment

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

You can import PoolType directly fromdjango_topics.models since you've already imported it there from the sub-file


def delete_all_data():
print("Delete data")
Topic.pools.through.objects.all().delete()
Copy link
Contributor

Choose a reason for hiding this comment

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

The explicit deletion of the through model records is not strictly needed.
On intermediate through models in a m2m relationship, the default behavior when deleting either of the related models is to delete the associated records in the through model as well (the on_delete parameter of the ForeignKey defaults to CASCADE)
Your call if it's clearer to keep it in or not.

@@ -50,7 +50,7 @@ class Header extends Component {
{ Sefaria._siteSettings.TORAH_SPECIFIC ?
<a className="home" href="/" >{logo}</a> : null }
<a href="/texts" className="textLink"><InterfaceText context="Header">Texts</InterfaceText></a>
<a href="/topics" className="textLink"><InterfaceText>Topics</InterfaceText></a>
<a href="/django_topics" className="textLink"><InterfaceText>Topics</InterfaceText></a>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this (and all the diffs below changing /topics to /django_topics) might be a mistake... It doesn't seem like we set any route for this URL.

Maybe an errant find/replace?

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.

2 participants