Merge branch 'modularization-main' into feature/sc-26951/implement-he…

…ro-banner

.github/pull_request_template.md

@@ -0,0 +1,8 @@
+## Description
+_A brief description of the PR_
+
+## Code Changes
+_The following changes were made to the files below_
+
+## Notes
+_Any additional notes go here_

build/ci/sandbox-values.yaml

@@ -44,7 +44,7 @@ monitor:
     tag: 
 secrets:
   localSettings: 
-    ref: local-settings-secrets-dev
+    ref: local-settings-secrets
   backupManager: 
     ref: backup-manager
   slackWebhook: 

helm-chart/sefaria-project/templates/configmap/local-settings-file.yaml

@@ -47,6 +47,7 @@ data:
     ADMINS = (
         ('Sefaria Developers', '[email protected]'),
     )
+    ADMIN_PATH = os.getenv("SEFARIA_ADMIN_PATH")
 
     MANAGERS = ADMINS
 

reader/views.py

@@ -78,7 +78,7 @@
 from sefaria.utils.user import delete_user_account
 from django.core.mail import EmailMultiAlternatives
 from babel import Locale
-from sefaria.helper.topic import update_topic, update_topic_titles
+from sefaria.helper.topic import update_topic
 from sefaria.helper.category import update_order_of_category_children, check_term
 
 if USE_VARNISH:
@@ -1242,6 +1242,7 @@ def edit_text(request, ref=None, lang=None, version=None):
     })
 
 @ensure_csrf_cookie
+@staff_member_required
 @sanitize_get_params
 def edit_text_info(request, title=None, new_title=None):
     """
@@ -2437,9 +2438,9 @@ def _internal_do_post(request, update, cat, uid, **kwargs):
         else:
             return jsonResponse({"error": "Only Sefaria Moderators can add or delete categories."})
 
-        j = request.POST.get("json")
+        j = request.body
         if not j:
-            return jsonResponse({"error": "Missing 'json' parameter in post data."})
+            return jsonResponse({"error": "Missing data in POST request."})
         j = json.loads(j)
         update = int(request.GET.get("update", False))
         new_category = Category().load({"path": j["path"]})
@@ -3108,7 +3109,9 @@ def add_new_topic_api(request):
         data = json.loads(request.POST["json"])
         isTopLevelDisplay = data["category"] == Topic.ROOT
         t = Topic({'slug': "", "isTopLevelDisplay": isTopLevelDisplay, "data_source": "sefaria", "numSources": 0})
-        update_topic_titles(t, **data)
+        titles = data.get('titles')
+        if titles:
+            t.set_titles(titles)
         t.set_slug_to_primary_title()
         if not isTopLevelDisplay:  # not Top Level so create an IntraTopicLink to category
             new_link = IntraTopicLink({"toTopic": data["category"], "fromTopic": t.slug, "linkType": "displays-under", "dataSource": "sefaria"})
@@ -3270,7 +3273,7 @@ def topic_ref_api(request, tref):
 
 @staff_member_required
 def reorder_sources(request):
-    sources = json.loads(request.POST["json"]).get("sources", [])
+    sources = json.loads(request.body).get("sources", [])
     slug = request.GET.get('topic')
     lang = 'en' if request.GET.get('lang') == 'english' else 'he'
     return jsonResponse(update_order_of_topic_sources(slug, sources, request.user.id, lang=lang))

requirements.txt

@@ -1,15 +1,18 @@
 Appium-Python-Client==1.2.0
-apscheduler==3.6.
+Cerberus
+PyJWT==1.7.1 # pinned b/c current version 2.0.0 breaks simplejwt. waiting for 2.0.1
+apscheduler==3.6.*
+babel
 bleach==1.4.2
 boto3==1.16.6
 bs4==0.0.1
+celery[redis]
 convertdate==2.2.2
 cython==0.29.14
 dateutils==0.6.12
 datrie==0.8.2
 deepdiff==3.3.0
 diff_match_patch==20200713
-django_mobile==0.7.0
 django-anymail==7.2.*
 django-debug-toolbar==2.2 # not used in prod
 django-recaptcha==2.0.6
@@ -18,18 +21,22 @@ django-structlog==1.6.2
 django-user-agents==0.4.0
 django-webpack-loader==1.4.1
 django==1.11.*
+django_mobile==0.7.0
 djangorestframework @ https://github.com/encode/django-rest-framework/archive/3.11.1.tar.gz
 djangorestframework_simplejwt==3.3.0
-PyJWT==1.7.1 # pinned b/c current version 2.0.0 breaks simplejwt. waiting for 2.0.1
+dnspython~=2.5.0
 elasticsearch==8.8.2
-git+https://github.com/Sefaria/[email protected]#egg=elasticsearch-dsl
-git+https://github.com/Sefaria/[email protected]#egg=sefaria_llm_interface&subdirectory=app/llm_interface
 geojson==2.5.0
 geopy==2.3.0
 gevent==20.12.0; sys_platform != 'darwin'
+git+https://github.com/Sefaria/[email protected]#egg=sefaria_llm_interface&subdirectory=app/llm_interface
+git+https://github.com/Sefaria/[email protected]#egg=elasticsearch-dsl
 google-api-python-client==1.12.5
+google-auth-oauthlib==0.4.2
+google-auth==1.24.0
 google-cloud-logging==1.15.1
 google-cloud-storage==1.32.0
+google-re2
 gunicorn==20.0.4
 html5lib==0.9999999
 httplib2==0.18.1
@@ -38,37 +45,31 @@ jedi==0.18.1 # Ipython was previosuly pinned at 7.18 because Jedi 0.18 broke it.
 jsonpickle==1.4.1
 lxml==4.6.1
 mailchimp==2.0.9
-google-auth==1.24.0
-google-auth-oauthlib==0.4.2
 p929==0.6.1
 pathos==0.2.6
-pillow==8.0.1; sys_platform == 'linux'
 pillow==10.0.1; sys_platform != 'linux'
-psycopg2==2.8.6
+pillow==8.0.1; sys_platform == 'linux'
+psycopg2==2.8.6 #for dev: psycopg2-binary==2.8.6
 py2-py3-django-email-as-username==1.7.1
 pymongo==3.12.*
 pytest==6.1.1
+python-bidi
 pytz
 pyyaml==6.0.1
 rauth==0.7.3
 redis==3.5.3
 regex==2020.10.23
+requests
 roman==3.3
 selenium==3.141.0
+sentry-sdk==1.26.0
 tqdm==4.51.0
 ua-parser==0.10.0
 undecorated==0.3.0
 unicodecsv==0.14.1
 unidecode==1.1.1
 user-agents==2.2.0
-sentry-sdk==1.26.0
-babel
-python-bidi
-requests
-Cerberus
-celery[redis]
-google-re2
-dnspython~=2.5.0
+
 
 #opentelemetry-distro
 #opentelemetry-exporter-otlp

sefaria/helper/tests/topic_test.py

@@ -67,6 +67,17 @@ def author_root():
 	yield {"topic": t, "link": l}
 	t.delete()
 
+@pytest.fixture(autouse=True, scope='module')
+def some_topic():
+	t = Topic({'slug': "abcd_test", "data_source": "sefaria", "numSources": 0})
+	title = "title in English"
+	he_title = "כותרת בעברית"
+	t.add_primary_titles(title, he_title)
+	t.set_slug_to_primary_title()
+	t.save()
+	yield t
+	t.delete()
+
 
 @pytest.fixture(autouse=True, scope='module')
 def actual_author(author_root):
@@ -86,18 +97,24 @@ def actual_author(author_root):
 
 def test_title_and_desc(author_root, actual_author, root_with_self_link, child_of_root_with_self_link, grandchild_of_root_with_self_link):
 	for count, t in enumerate([author_root, actual_author, root_with_self_link, child_of_root_with_self_link, grandchild_of_root_with_self_link]):
-		new_values = {"title": f"new title {count+1}",
-					  "altTitles": {"en": [f"New Alt title {count+1}"], "he": [f"New He Alt Title {count+1}"]},
-					  "heTitle": f"new hebrew title {count+1}", "description": {"en": f"new desc", "he": "new hebrew desc"}}
+		en_primary_title = {"text": f"new title {count+1}", "primary": True, "lang": 'en'}
+		he_primary_title = {"lang": "he", "text": f"new hebrew title {count+1}", "primary": True}
+
+		en_alt_title = {"lang": "en",  "text": f"New Alt title {count+1}"}
+		he_alt_title = {"lang": "he", "text": f"New He Alt Title {count+1}"}
+
+		new_values = {"titles": [en_primary_title, en_alt_title, he_alt_title, he_primary_title],
+					  "description": {"en": f"new desc", "he": "new hebrew desc"}}
 		topic.update_topic(t["topic"], **new_values)
 		assert t["topic"].description == new_values["description"]
-		assert t["topic"].get_primary_title('he') == new_values['heTitle']
-		assert t["topic"].get_titles('en') == [new_values["title"]]+new_values["altTitles"]['en']
+		assert t["topic"].get_primary_title('he') == he_primary_title['text']
+		assert t["topic"].get_titles('en') == [en_primary_title['text'], en_alt_title['text']]
 
 def test_author_root(author_root, actual_author):
-	new_values = {"category": "authors", "title": actual_author["topic"].get_primary_title('en'),
-				  "heTitle": actual_author["topic"].get_primary_title('he'),
-				  "birthPlace": "Kyoto, Japan", "birthYear": 1300}
+	new_values = {"category": "authors", "titles": [
+		{'text': actual_author["topic"].get_primary_title('en'), "lang": 'en', 'primary': True},
+		{"text": actual_author["topic"].get_primary_title('he'), "lang": 'he', 'primary': True}],
+	  	"birthPlace": "Kyoto, Japan", "birthYear": 1300}
 	assert Place().load({'key': new_values["birthPlace"]}) is None
 	topic.update_topic(actual_author["topic"], **new_values)
 	assert Place().load({'key': new_values["birthPlace"]})
@@ -111,18 +128,19 @@ def test_change_categories_and_titles(author_root, root_with_self_link):
 	orig_tree_from_root_with_self_link = library.get_topic_toc_json_recursive(root_with_self_link["topic"])
 	orig_trees = [orig_tree_from_normal_root, orig_tree_from_root_with_self_link]
 	roots = [author_root["topic"], root_with_self_link["topic"]]
-	orig_titles = [roots[0].get_primary_title('en'), roots[1].get_primary_title('en')]
-	orig_he_titles = [roots[0].get_primary_title('he'), roots[1].get_primary_title('he')]
+	orig_titles = [{'text': roots[0].get_primary_title('en'), 'lang':'en', 'primary': True}, {'text': roots[1].get_primary_title('en'), 'lang':'en', 'primary': True}]
+	orig_he_titles = [{'text': roots[0].get_primary_title('he'), 'lang':'he', 'primary': True}, {'text': roots[1].get_primary_title('he'), 'lang':'he', 'primary': True}]
 	for i, root in enumerate(roots):
 		other_root = roots[1 - i]
-		topic.update_topic(root, title=f"fake new title {i+1}", heTitle=f"fake new he title {i+1}", category=other_root.slug)  # move root to be child of other root
+		topic.update_topic(root, titles=[{'text': f"fake new title {i+1}", 'lang': 'he', 'primary': True},
+										 {'text': f"fake new he title {i+1}", 'lang': 'he', 'primary': True}], category=other_root.slug)  # move root to be child of other root
 		new_tree = library.get_topic_toc_json_recursive(other_root)
 		assert new_tree != orig_trees[i]  # assert that the changes in the tree have occurred
-		assert root.get_titles('en') != [orig_titles[i]]
-		assert root.get_titles('he') != [orig_he_titles[i]]
-		topic.update_topic(root, title=orig_titles[i], heTitle=orig_he_titles[i], category=Topic.ROOT)  # move it back to the main menu
-		assert root.get_titles('en') == [orig_titles[i]]
-		assert root.get_titles('he') == [orig_he_titles[i]]
+		assert root.get_titles('en') != [orig_titles[i]['text']]
+		assert root.get_titles('he') != [orig_he_titles[i]['text']]
+		topic.update_topic(root, titles=[orig_titles[i], orig_he_titles[i]], category=Topic.ROOT)  # move it back to the main menu
+		assert root.get_titles('en') == [orig_titles[i]['text']]
+		assert root.get_titles('he') == [orig_he_titles[i]['text']]
 
 
 	final_tree_from_normal_root = library.get_topic_toc_json_recursive(roots[0])
@@ -178,3 +196,19 @@ def test_calculate_approved_review_state(current, requested, was_ai_generated, m
 ])
 def test_get_merged_descriptions(current, requested, merged):
 	assert topic._get_merged_descriptions(current, requested) == merged
+
+
+def test_update_topic(some_topic):
+	topic.update_topic(some_topic, titles=[{"text": "Tamar", "lang": "en", "primary": True},
+							 {"text": "תמר", "lang": "he", "primary": True, "disambiguation": "יהודה"}])
+	assert some_topic.titles == [{"text": "Tamar", "lang": "en", "primary": True},
+						 {"text": "תמר", "lang": "he", "primary": True, "disambiguation": "יהודה"}]
+
+	topic.update_topic(some_topic, description={"en": "abcdefg"})
+	assert some_topic.description == {"en": "abcdefg"}
+
+	with pytest.raises(Exception):
+		topic.update_topic(some_topic, titles=[{"a": "Tamar", "b": "en"},
+								 {"c": "תמר", "lang": "d", "disambiguation": "יהודה"}])
+	with pytest.raises(Exception):
+		topic.update_topic(some_topic, slug='abc')

sefaria/helper/topic.py

@@ -1070,18 +1070,6 @@ def topic_change_category(topic_obj, new_category, old_category="", rebuild=Fals
         rebuild_topic_toc(topic_obj, category_changed=True)
     return topic_obj
 
-def update_topic_titles(topic, title="", heTitle="", **kwargs):
-    new_primary = {"en": title, "he": heTitle}
-    for lang in ['en', 'he']:   # first remove all titles and add new primary and then alt titles
-        for title in topic.get_titles(lang):
-            topic.remove_title(title, lang)
-        topic.add_title(new_primary[lang], lang, True, False)
-        if 'altTitles' in kwargs:
-            for title in kwargs['altTitles'][lang]:
-                topic.add_title(title, lang)
-    return topic
-
-
 def update_authors_place_and_time(topic, dataSource='learning-team-editing-tool', **kwargs):
     # update place info added to author, then update year and era info
     if not hasattr(topic, 'properties'):
@@ -1097,11 +1085,11 @@ def update_properties(topic_obj, dataSource, k, v):
 
 def update_author_era(topic_obj, dataSource='learning-team-editing-tool', **kwargs):
     for k in ["birthYear", "deathYear"]:
-        if k in kwargs.keys():   # only change property value if key exists, otherwise it indicates no change
+        if kwargs.get(k, False):   # only change property value if key exists, otherwise it indicates no change
             year = kwargs[k]
             update_properties(topic_obj, dataSource, k, year)
 
-    if 'era' in kwargs.keys():    # only change property value if key is in data, otherwise it indicates no change
+    if kwargs.get('era', False):    # only change property value if key is in data, otherwise it indicates no change
         prev_era = topic_obj.properties.get('era', {}).get('value')
         era = kwargs['era']
         update_properties(topic_obj, dataSource, 'era', era)
@@ -1110,46 +1098,49 @@ def update_author_era(topic_obj, dataSource='learning-team-editing-tool', **kwar
     return topic_obj
 
 
-def update_topic(topic, **kwargs):
+def update_topic(topic, titles=None, category=None, origCategory=None, categoryDescritpion=None, description=None,
+                 birthPlace=None, deathPlace=None, birthYear=None, deathYear=None, era=None,
+                 rebuild_toc=True, manual=False, image=None, **kwargs):
     """
-    Can update topic object's title, hebrew title, category, description, and categoryDescription fields
+    Can update topic object's titles, category, description, and categoryDescription fields
     :param topic: (Topic) The topic to update
-    :param **kwargs can be title, heTitle, category, description, categoryDescription, and rebuild_toc where `title`, `heTitle`,
-         and `category` are strings. `description` and `categoryDescription` are dictionaries where the fields are `en` and `he`.
+    :param **kwargs can be titles, category, description, categoryDescription, and rebuild_toc where `titles` is a list
+     of title objects as they are represented in the database, and `category` is a string. `description` and `categoryDescription` are dictionaries where the fields are `en` and `he`.
          The `category` parameter should be the slug of the new category. `rebuild_topic_toc` is a boolean and is assumed to be True
     :return: (model.Topic) The modified topic
     """
     old_category = ""
     orig_slug = topic.slug
-    update_topic_titles(topic, **kwargs)
-    if kwargs.get('category') == 'authors':
-        topic = update_authors_place_and_time(topic, **kwargs)
 
-    if 'category' in kwargs and kwargs['category'] != kwargs.get('origCategory', kwargs['category']):
+    if titles:
+        topic.set_titles(titles)
+    if category == 'authors':
+        topic = update_authors_place_and_time(topic, birthPlace=birthPlace, birthYear=birthYear, deathPlace=deathPlace, deathYear=deathYear, era=era)
+
+    if category and origCategory and origCategory != category:
         orig_link = IntraTopicLink().load({"linkType": "displays-under", "fromTopic": topic.slug, "toTopic": {"$ne": topic.slug}})
         old_category = orig_link.toTopic if orig_link else Topic.ROOT
-        if old_category != kwargs['category']:
-            topic = topic_change_category(topic, kwargs["category"], old_category=old_category)  # can change topic and intratopiclinks
+        if old_category != category:
+            topic = topic_change_category(topic, category, old_category=old_category)  # can change topic and intratopiclinks
 
-    if kwargs.get('manual', False):
+    if manual:
         topic.data_source = "sefaria"  # any topic edited manually should display automatically in the TOC and this flag ensures this
         topic.description_published = True
 
-    if "description" in kwargs or "categoryDescription" in kwargs:
-        topic.change_description(kwargs.get("description", None), kwargs.get("categoryDescription", None))
+    if description or categoryDescritpion:
+        topic.change_description(description, categoryDescritpion)
 
-    if "image" in kwargs:
-        image_dict = kwargs["image"]
-        if image_dict["image_uri"] != "":
-            topic.image = kwargs["image"]
+    if image:
+        if image["image_uri"] != "":
+            topic.image = image
         elif hasattr(topic, 'image'):
             # we don't want captions without image_uris, so if the image_uri is blank, get rid of the caption too
             del topic.image
 
     topic.save()
 
-    if kwargs.get('rebuild_topic_toc', True):
-        rebuild_topic_toc(topic, orig_slug=orig_slug, category_changed=(old_category != kwargs.get('category', "")))
+    if rebuild_toc:
+        rebuild_topic_toc(topic, orig_slug=orig_slug, category_changed=(old_category != category))
     return topic
 
 
@@ -1320,12 +1311,13 @@ def delete_ref_topic_link(tref, to_topic, link_type, lang):
     if link is None:
         return {"error": f"A learning-team link between {tref} and {to_topic} doesn't exist. If you are trying to delete a non-learning-team link, reach out to the engineering team."}
 
-    if lang in link.order.get('availableLangs', []):
-        link.order['availableLangs'].remove(lang)
     if lang in link.order.get('curatedPrimacy', []):
         link.order['curatedPrimacy'].pop(lang)
+    if lang in getattr(link, 'descriptions', {}):
+        link.descriptions.pop(lang)
 
-    if len(link.order.get('availableLangs', [])) > 0:
+    # Note, using curatedPrimacy as a proxy here since we are currently only allowing deletion of learning-team links.
+    if len(link.order.get('curatedPrimacy', [])) > 0:
         link.save()
         return {"status": "ok"}
     else:   # deleted in both hebrew and english so delete link object

sefaria/local_settings_example.py

@@ -53,6 +53,8 @@
 ADMINS = (
      ('Your Name', '[email protected]'),
 )
+ADMIN_PATH = 'somethingsomething' #This will be the path to the admin site, locally it can also be 'admin'
+
 PINNED_IPCOUNTRY = "IL" #change if you want parashat hashavua to be diaspora.
 
 MONGO_REPLICASET_NAME = None # If the below is a list, this should be set to something other than None.