MA-1882 adding course api to regex

[sanfordstudent]

@nasthagiri @jcdyer

Fixes https://openedx.atlassian.net/browse/MA-1882

This may not be the cleanest way to do this regex.

[nasthagiri]

Sure. This works. Alternatively, you can search for and match on the last "courses/" that is found.

Please add a unit test.

[sanfordstudent]

I realized this approach doesn't accommodate /blocks url's with mongo-style course id's-

/api/courses/v1/blocks/edx/maths/2020/......

yields a course id of v1/blocks/edx, which we certainly don't want.

I'm going to have to rework this.

[sanfordstudent]

Now updated with more comprehensive tests and a regex that excludes /v*/blocks routes explicitly.

[nasthagiri]

@mulby Can you take at a look at this PR? Is this what the analytics team intended for annotating events with course_ids? The URL matching seems a bit fragile to me. But I haven't thought of any better alternatives. What do you think?

[sanfordstudent]

I'm going to try to generalize the regex to exclude any route that includes "v*/{}" where {} is not courses.

[mulby]

@nasthagiri - agreed it's very fragile, note that I don't think analytics is the only part of the system that relies on this. Also, I don't think this is the only regex of this kind in the system.

I'm not sure if there is a better way to do this? The only thing I can think of is to somehow run the django URL resolving -> view resolving engine on the URL and looking for an extracted course_id. If we were diligent about always naming the parameter "course_id" or "course_key" we could identify the view and look for parameters that had those names?

I'm not sure if there is a more idiomatic way to handle something like this in Django.

Note that the analytics system does provide a context manager that allows you to override this automagically parsed course_id, you just have to do something like this:

with tracker.context('my_ctx_override', {'course_id': real_course_id}):
    tracker.emit(...)

[sanfordstudent]

jenkins run bokchoy

[sanfordstudent]

jenkins run js

[nasthagiri]

update docstring since it's no longer /blocks specific.

[nasthagiri]

👍

[sanfordstudent]

adding @jcdyer for review.

[jcdyer]

What is the + at the end of (.*?/courses/)+ intended to do? It looks like you're expecting .*?/courses/ to show up multiple times, but that seems weird. Maybe the comment should include some examples of URLs that we intend to match with this.

[sanfordstudent]

It can show up multiple times, yes. The tests include URLs such as "/api/courses/v1/courses/course-v1:edX+maths+2020" that exhibit this.

[jcdyer]

But you already handle that case by specifying that you want to match /courses/ not followed by v[0-9]+, right?

[sanfordstudent]

Good point!

[jcdyer]

Other than deduplication of tests, 👍