Mjames/sol 174

[martynjames]

@dino-cikatic - can you do a first-pass review for these changes
@dsego - It would be nice if you can review too - but seeing as you performed some of the code, you can't be the official reviewer

[martynjames]

Jenkins test this please

[dino-cikatic]

I don't think expected is an argument here, seems like return value is bool result of search operation.

[martynjames]

@dino-cikatic; @dsego - Some updates have been made. @e-kolpakov - This should be good to go and includes moving the indexing code out of common and into cms... now that it is triggered by a signal instead of being called directly from within the modulestore code.

Update: Looks like a couple of errors got introduced with rebase - but worth having you guys look at it in the meantime

[e-kolpakov]

@martynjames overall looks good, but some tests are broken

[dino-cikatic]

Couple of typos.

[martynjames]

@dino-cikatic, @e-kolpakov - Addressed the problems resulting from rebase, and feedback items. Hoping to see if I can get a thumbs up from either of you to move onto 2nd review phase. If there are any other items you desire me to change/update, please let me know.

[e-kolpakov]

@martynjames I see what you're doing here, but it took me about 15 minutes before I realized why it is needed and why it still recursively calls index_item for items that are not actually indexed. Might be worth commenting at least.

[martynjames]

added at initialization of indexed_items

[martynjames]

jenkins test this please

[martynjames]

@clintonb & @ormsbee - We've gone through a pretty thorough first-pass review, so I think I'm ready for your input on here (pending passing tests - last time through I had one failure that appears to be happening for others too).

This represents the changes for responding to the course_published signal:

• triggers celery task from signal
• Rework of indexing code to always perform it from the course level
• Moves indexing code from common to cms, now that its execution is signaled
• Fixes an issues with deleting items from the index

[ormsbee]

Why are we instantiating an indexer object here? It looks like we never actually store associated state with this object (e.g. these are really just class methods)?

[e-kolpakov]

@ormsbee there's a related PR that adds content libraries indexing. It was submitted earlier than this one, but there is an irremovable defect with tests: search indexers used to live in common/lib, which should not depend on any django features - but search indexers implicitly depends on SEARCH_ENGINE setting. So, since this PR significantly changes and improves indexing and fixes that test issue, it was decided #7448 is suspended until this PR is merged.

So, having indexers a classmethods or instance methods was my suggestion to simplify rebasing #7448 on top of this. @martynjames likely chosen instance methods because I used instance methods in #7448 (but he definitely knows better). If it makes any difference, I used instance methods in #7448 because I personally find classes with instance methods induce less coupling between "provider" and "client" code. Static and class methods explicitly mentions implementing class, while instance methods can be called on any class implementing certain inteface, thus allowing swapping implementations easier than with static/class methods.

[martynjames]

I changed them to instance methods because I was under the impression that that is what you required - they were already class methods (given that a static method is really just a class method that does not have a reference to the class object). If no one objects, I'll change them back - class methods would be my preference.

[martynjames]

jenkins test this please

[e-kolpakov]

@martynjames what's the reason for converting course_key to unicode here, than reconstructing it from string in the task itself. I'm asking this because I'm about adding another task and signla to index library, and it seems a bit weird to replicate this.

[martynjames]

Good question, and I have a good answer - this is the point at which the remoting to the celery process happens. Unfortunately, if you are running inline everything works just fine, but on a real call (async celery process) the arguments here need to be serialized to JSON and course_key is not serializable; but a string is!

[e-kolpakov]

@martynjames thank you for clarification - actually I suspected something like this, but decided it would be better to ask if that's true.

[e-kolpakov]

@martynjames actually, there might be another problem. I've just uploaded my #7641 to sandbox and it fails here:

Sent course_published signal to <function listen_for_course_publish at 0x4951c08> with kwargs {'course_key': CourseLocator(u'Arbi', u'101', u'2015', None, None)}. Response was: datetime.datetime(2015, 4, 9, 16, 13, 55, 457562, tzinfo=<UTC>) is not JSON serializable

In two words, datetime objects are not json-serializable as well (not sure if it applies to all python versions though). So it might need to be converted to unicode and parsed back.

[martynjames]

Thank you - I will address immediately

[martynjames]

@clintonb - didn't know if you had anything else to add here?

[clintonb]

@martynjames I've been out of the loop a bit too long with vacation and Q4 planning. Please move forward without me.

[martynjames]

Thanks @clintonb!

[martynjames]

@ormsbee - hoping to see if you are ready for thumbs up on this one...

[martynjames]

@ormsbee - I made the changes that we discussed yesterday. Test run has passed - thanks!

[ormsbee]

👍 -- please just squash the commits

[martynjames]

@ormsbee, @BenjiLee, @mattdrayer - Had to make a small tweak to one file - import problem shows up when workers startup with the error - If you guys can quickly review the last change I would appreciate it

Example error in celery process startup:

...
  File "/edx/app/edxapp/venvs/edxapp/src/edx-val/edxval/views.py", line 57, in VideoList
    from edxval.serializers import VideoSerializer
ImportError: cannot import name VideoSerializer

[BenjiLee]

If referring to ae172b4, that change should be fine as thats the only place copy_course_videos is used in edx-platform.

[martynjames]

Yes, thanks @BenjiLee - can I assume that's a +1?

[BenjiLee]

👍

[ormsbee]

👍

[BenjiLee]

@martynjames Could you do a course rerun to verify that copy_course_rerun works via the celery process?

[martynjames]

@BenjiLee - Luckily I had a sandbox ready.... re-run appeared to work just fine - in particular, the videos appeared to still be present within the rerun course

[BenjiLee]

LGTM 👍