Fix #2553: Change the "index all explorations" job in the admin dashb… #3831

LanJosh · 2017-08-30T21:07:34Z

…oard to an "index all activities" job

…dashboard to an "index all activities" job

seanlip

Thanks @LanJosh, this is looking good! Just a few notes.

seanlip · 2017-08-30T21:10:31Z

core/domain/collection_services.py

 def clear_search_index():
    """Clears the search index.

    WARNING: This runs in-request, and may therefore fail if there are too
    many entries in the index.
    """
-    search_services.clear_index(SEARCH_INDEX_COLLECTIONS)
+    gae_search_services.clear_index(SEARCH_INDEX_COLLECTIONS)


Oh, this is interesting. How feasible do you think it would be to move the functions which require gae_search_services to search_services, so that the latter is the only thing interfacing with gae_search_services?

seanlip · 2017-08-30T21:12:44Z

core/domain/collection_services.py

-        if _should_index(collection)
-    ], SEARCH_INDEX_COLLECTIONS)
+    collection_summaries = get_collection_summaries_matching_ids(collection_ids)
+    search_services.index_collection_summaries(collection_summaries)


Should probably filter the list to remove "None" results, right, since the previous function can return None?

seanlip · 2017-08-30T21:13:48Z

core/domain/search_services.py

+
+"""Commands for manipulating search rank"""
+
+from core.platform import models


Alphabetize.

seanlip · 2017-08-30T21:14:03Z

core/domain/search_services.py

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Commands for manipulating search rank"""


Add period at end.

This module does more than just manipulate search rank, right?

codecov-io · 2017-08-30T22:33:16Z

Codecov Report

Merging #3831 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #3831   +/-   ##
========================================
  Coverage    46.02%   46.02%           
========================================
  Files          279      279           
  Lines        20903    20903           
  Branches      3278     3278           
========================================
  Hits          9620     9620           
  Misses       11283    11283

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 98a592b...b6da70c. Read the comment docs.

1995YogeshSharma

Looks good. Just a few more comments.

Also, please reply to each review comments after doing changes. This helps keeping track of which have been addressed. Thanks !

1995YogeshSharma · 2017-08-31T20:35:56Z

core/controllers/cron.py

@@ -96,14 +96,14 @@ def get(self):
        job_class.enqueue(job_class.create_new())


-class CronExplorationSearchRankHandler(base.BaseHandler):
+class CronActivitySearchRankHandler(base.BaseHandler):
    """Handler for computing exploration search ranks."""


should this be activity here instead of exploration?

1995YogeshSharma · 2017-08-31T20:36:53Z

core/domain/activity_jobs_one_off.py

@@ -0,0 +1,47 @@
+# coding: utf-8
+#
+# Copyright 2014 The Oppia Authors. All Rights Reserved.


2014 --> 2017

1995YogeshSharma · 2017-08-31T20:49:45Z

core/domain/search_services.py

@@ -0,0 +1,301 @@
+# coding: utf-8
+#
+# Copyright 2014 The Oppia Authors. All Rights Reserved.


2014 --> 2017

1995YogeshSharma · 2017-08-31T20:51:47Z

core/domain/search_services.py

+    be indexed for further queries or not.
+
+    Args:
+        exp: Exploration. Exploration domain object.


argument is exp_summary.

1995YogeshSharma · 2017-08-31T20:52:56Z

core/domain/search_services.py

+    search queries.
+
+    Args:
+        exp_summary: ExpSummaryModel.


Add a brief description of the argument. Look in the function below here.

1995YogeshSharma · 2017-08-31T20:55:04Z

core/domain/search_services.py

+            summary object to be converted.
+
+    Returns:
+        The search dict of the collection domain object.


Add the type of object returned.
dict. The search dict ...

1995YogeshSharma · 2017-08-31T20:57:22Z

core/domain/search_services.py

+    Args:
+        query_string: str. The query string to search for.
+        limit: int. The maximum number of results to return.
+        sort: str. A string indicating how to sort results. This should be a


str or None

1995YogeshSharma · 2017-08-31T20:59:43Z

core/domain/search_services.py

+
+    Args:
+        query_string: str. the query string to search for.
+        sort: str. This indicates how to sort results. This should be a string


str or None. Ditto for cursor.

1995YogeshSharma · 2017-08-31T21:00:20Z

core/domain/search_services.py

+            name to sort on. When this is None, results are returned based on
+            their ranking (which is currently set to the same default value
+            for all collections).
+        limit: int. the maximum number of results to return.


Shift it above the sort. Follow the order in arguments.

1995YogeshSharma · 2017-08-31T21:12:22Z

core/domain/search_services.py

+"""Commands for operating on the search status of activities."""
+
+from core.platform import models
+from core.domain import rights_manager


Alphabetize.
core.domain should come before core.platform

…ties-job

LanJosh · 2017-08-31T23:15:28Z

Hi @1995YogeshSharma, mind taking another look?

1995YogeshSharma

Just one small change. Otherwise, LGTM!
Thanks!

1995YogeshSharma · 2017-09-01T00:14:45Z

core/domain/search_services.py

@@ -50,7 +50,7 @@ def _exp_summary_to_search_dict(exp_summary):
    be indexed for further queries or not.

    Args:
-        exp: Exploration. Exploration domain object.
+        exp: ExpSummaryModel. ExplorationSummary domain object.


exp --> exp_summary (name of arg)

pranavsid98 · 2017-09-05T01:39:38Z

Hi @1995YogeshSharma. Can this be merged?

1995YogeshSharma · 2017-09-05T02:05:35Z

Yes. I think it's good to go.

pranavsid98 · 2017-09-05T02:16:43Z

Hi @tjiang11 . I don't think I have merge permissions. Yogesh says its good to go, can you merge this?

…t-m2 * upstream/develop: (27 commits) Fixes 3687: batch call for exploration and exploration rights object together. (oppia#3815) Fix part oppia#3400 Objective field directive (oppia#3740) Fix oppia#3800 Dropdown menu of the ABOUT in navigation bar (oppia#3855) Fix oppia#3295 Save draft change list to local storage. (oppia#3584) Disable learner dashboard feedback updates send button when no text entered. (oppia#3860) Disable save button while uploading audio to server; add Spanish support to audio languages; add audio loading message to learner view (oppia#3856) Fix oppia#3834: Introduce backend event models to record statistics (oppia#3841) Fix oppia#3852: Fix failing e2e test in develop (oppia#3853) Fix oppia#3826: Move validatorsService from app.js to ValidatorsService.js (oppia#3847) Update the max RAND limit for ID generation to fit within 32 bits. (oppia#3843) Fix oppia#3404: Add default placeholder text for mobile devices (oppia#3845) Fix oppia#2553: Change the "index all explorations" job in the admin dashb… (oppia#3831) Add I18N to give up button; change tooltip text. (oppia#3844) Fix oppia#3721: Update the release_info script to use LCA and the most recent release tag in order to compile the list of changes, rather than relying on git describe. (oppia#3838) show_warning_only_on_button_click (oppia#3833) Deprecate splash page experiment (oppia#3829) Fixes for end to end working between Oppia and Oppia-ml. (oppia#3824) Fix 'Empty path passed in method' error on the collection page. (oppia#3827) Learner dashboard 3.2 (oppia#3759) 📝 Fix oppia#3789 :Space out the profile drop down icons (oppia#3789). (oppia#3817) ...

Fix oppia#2553: Change the "index all explorations" job in the admin …

9c57e8d

…dashboard to an "index all activities" job

LanJosh requested a review from 1995YogeshSharma August 30, 2017 21:08

LanJosh assigned 1995YogeshSharma Aug 30, 2017

seanlip reviewed Aug 30, 2017

View reviewed changes

Address review comments

8b08740

1995YogeshSharma suggested changes Aug 31, 2017

View reviewed changes

1995YogeshSharma reviewed Aug 31, 2017

View reviewed changes

LanJosh added 2 commits August 31, 2017 17:51

Address review comments

fc26372

Merge remote-tracking branch 'upstream/develop' into index-all-activi…

9ce82ef

…ties-job

1995YogeshSharma approved these changes Sep 1, 2017

View reviewed changes

Update docstring to correct parameter name

b6da70c

tjiang11 merged commit dd4d7c3 into oppia:develop Sep 5, 2017

LanJosh deleted the index-all-activities-job branch September 18, 2017 19:07


		"""Commands for manipulating search rank"""

		from core.platform import models

Fix #2553: Change the "index all explorations" job in the admin dashb… #3831

Fix #2553: Change the "index all explorations" job in the admin dashb… #3831

Conversation

LanJosh commented Aug 30, 2017

seanlip 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

Choose a reason for hiding this comment

codecov-io commented Aug 30, 2017 • edited Loading

Codecov Report

1995YogeshSharma left a comment • edited Loading

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

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

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

LanJosh commented Aug 31, 2017

1995YogeshSharma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavsid98 commented Sep 5, 2017

1995YogeshSharma commented Sep 5, 2017

pranavsid98 commented Sep 5, 2017

codecov-io commented Aug 30, 2017 •

edited

Loading

1995YogeshSharma left a comment •

edited

Loading