Populate the course goals user activity table when a user visits course-specific pages

[MatthewPiatetsky]

Populate user activity table for goals when users visit course-specific pages on web or the mobile apps
For this ticket: https://openedx.atlassian.net/browse/AA-905

Web seems to be covered by the courseware/tabs code, and the rest of the code is for the mobile app which calls several endpoints (and more will still need to be added).
Because there are a lot of endpoints that the mobile app hits, we are planning for the mobile team to add a call to the mobile apps to populate user activity. However, it may take a while until the code is written and the users have the new app version, so we're hooking into all these endpoints for the mobile app as a stopgap.
Once mobile adds their call, all of the calls for mobile should be removed.
They can be identified because they are using the check_if_mobile_app parameter.

TODOs:

• Manual Testing for all the mobile endpoints - I've added a waffle flag, so I will plan to test with the mobile apps from stage and production

The apis for mobile that I am trying to cover here are:

1. /api/courses/v2/blocks/
2. /api/course_home/v1/dates/{courseID}
3. api/mobile/v0.5/course_info/{course_id}/handouts
4. api/mobile/v0.5/course_info/{course_id}/updates
5. /api/discussion/v1/courses/{course_id}/
6. /api/discussion/v1/threads/
7. /api/discussion/v1/comments/
8. /api/discussion/v1/course_topics/{course_id}
9. /api/course_experience/v1/course_deadlines_info/course-v1:UPValenciaX+BSP101x+1T2021
10. /api/course_home/v1/dates/course-v1:UPValenciaX+BSP101x+1T2021
11. /api/discussion/v1/course_topics/course-v1:UPValenciaX+BSP101x+1T2021
12. /api/courseware/course/course-v1:UPValenciaX+BSP101x+1T2021
13. /xblock/block-v1:UPValenciaX+BSP101x+1T2021+type@html+block@f70a03cd2f67434abe52e9fdd785c0cf

[MatthewPiatetsky]

import currently here because it was causing problems for cms tests which i think weren't getting the goals migration or something - but will look into if i can avoid this

[MatthewPiatetsky]

api that mobile app will use for recording activity

[MatthewPiatetsky]

this is the only spot that populates user activity for web - all the other spots are temporary spots for mobile

[mikix]

Why not put this inside the method too? It can own all the things that would cause it to not log activity (like bad course, non-mobile app, etc) - and this waffle.

[MatthewPiatetsky]

i haven't actually tested some of the connection points with mobile locally, and there are a lot of calls, so i was being defensive here, because i didn't want one of these calls to trigger some exception before it had been thoroughly tested

[mikix]

Yeah I didn't say get rid of the waffle. I was suggesting bringing the waffle check inside the method. So you only have one spot checking it.

[MatthewPiatetsky]

yup, i didn't think that's what you meant
i meant that i put the flag on the outside since i haven't tested some of the endpoints from the mobile app, so i was concerned that the function call itself could trigger an exception (rather than the code inside the function)

[mikix]

I don't understand the concern - like you were worried you'd typo the function call? I think the linter would catch that. And you could still typo the waffle call to see if it was enabled.

[MatthewPiatetsky]

doesn't have to be a typo, could be that form.cleaned_data["course_id"] gives me a keyerror for example, although i guess if i was going to get import-related errors i'd still get them with the imports on the outside

[mikix]

1. check_if_mobile_app might be more self-documenting as only_if_mobile_app
2. We'd always have a request, I'd think. So why make it optional? And why not grab the user from that?

[MatthewPiatetsky]

1. sure will update the name
2. i wasn't passing the request for the one web spot, and all the mobile code will need to be removed soon, so at that point i think we can remove the request variable (i think better to also allow passing the user to avoid any masquerade issues)

[MatthewPiatetsky]

changed to only_if_mobile_app

[mikix]

I think as you add masquerade checks, you'd want the request object anyway.

[MatthewPiatetsky]

i've added the is_masquerade check, i don't think i need the request object for that?

[mikix]

Ah right - sorry. In my head the masquerade code went and got the current request from crum. It just grabs the masquerade settings from the user object.

It still feels cleaner to me to just require the request object, and then grab the user from there. This method is always in the context of a request because it is tracking user activity, which come in the form of requests. And it has logic inside that wants to check that request.

I know that that logic (is mobile request) only happens (and thus request is only required) if only_if_mobile_app is True - but that's an implicit relationship between arguments that I don't love.

So to me, it feels cleaner to say "I know the caller has a request. I kind of want to ensure the call has a request, actually. I can grab user and mobile info from that. And it removes implicit relationships between arguments."

Whereas maybe you're thinking "I like that the user argument is explicit, not implicit. And I don't want callers that don't care about mobile to need to handle the request object." Sound right?

If you are unpursuaded, can you at least make the implicit relationship between arguments clear in the function's docstring?

[MatthewPiatetsky]

i think my argument was that once we remove the mobile calls in the near future, we can remove the mobile argument and then it wouldn't make sense to pass the request only for the purposes of getting the user
i'd agree with you if i thought the mobile argument was going to stick around
but i've added documentation about the implicit relationship

[mikix]

Maybe consider using our TieredCache for this? I don't think you super care about the tiering here, but it's more in the vein of "use our abstraction-layer tooling, rather than directly calling the django cache"

Plus, that would make this code check is_found rather than for a falsy cache value (which isn't a problem here, but is a general pattern to avoid, for future engineers coming to this code and maybe introducing the bug unintentionally)

[MatthewPiatetsky]

you do make a good point about the falsey value, but i think we could counteract that by checking if the value is None?
i guess I can switch to TieredCache to ensure this bug doesn't happen - though maybe we should have a more basic cache utility that doesn't have the tiering but also allows you to check for the presence of a key?
as it looks like TieredCache comes with its own set of possible bugs (that shouldn't affect this code though)

[mikix]

What bugs?

[MatthewPiatetsky]

i was referencing this https://github.com/edx/edx-django-utils/blob/master/edx_django_utils/cache/README.rst#warning-when-storing-bools

anyways, i changed the cache to the TieredCache

[mikix]

This is a word you use a lot here, so maybe not worth changing. But I find the use of populate to be a bit misleading. Like, "record user activity" is vague but gets the idea across. "populate user activity" makes me wonder if it's fleshing out a table with more than one entry maybe? I didn't super care when it was all internal, but if we're using the word in an API url, maybe worth a think. (Populate isn't bad, I just feel it implies we're adding more than one entry of something.)

[MatthewPiatetsky]

hm, populate didn't bother me but I can change it to record if you think that's better

[MatthewPiatetsky]

changed to record

[mikix]

I mean, more opinions are good - see if anyone else has my gut read of "multiple items" - if so, probably good to change. But maybe it's just me that reads it like that.

[MatthewPiatetsky]

well i already changed everything to record, not going to change everything back to populate lol

[mikix]

So course_goals owns the django code for user activity and goals and what not. But for API endpoints, I'm not sure we want it here?

I intentionally added the API endpoint for unsubscribing by token inside the MFE's BFF djangoapp. It can change and evolve along wth the MFE - no promises are made to anyone but the MFE.

Likewise here, you know this is going to only be used by the mobile apps. Having it available in a generic spot like this raises questions like What is the contract for this API? Should it be versioned? Why isn't the MFE using this canonical API?

Might be better for everyone (especially Mobile's changing needs), if this were clearly part of the mobile world, and not trying to be a general purpose endpoint.

[MatthewPiatetsky]

good idea, i can add versioning
hm, i could name the endpoint something mobile specific - but I wonder if there is a use case in the future where something else could use it?
perhaps it being specific for the mobile apps should be noted in the comments?

well, originally i think the plan was just to hook into the backend to avoid the need for unnecessary extra requests.
this seems feasible by hooking into one spot for web, but for mobile there's a ton of calls, which is why we've asked mobile to add a request on their end, since this implementation wouldn't be maintainable long term for mobile
if mobile is removed and there's a single hook in for web i think that would be ok

[mikix]

I'm not just suggesting putting mobile in its name or adding a comment. I'm suggesting moving this endpoint to a djangoapp that is more suited - maybe mobile_api? Make this view a BFF for the mobile apps, rather than some generic versioned endpoint. (The lack of version was fine to my eyes - as long as its a BFF.)

[MatthewPiatetsky]

hm, well most mobile calls aren't going to the mobile_api, but sure i can make this specific to the mobile_api
you don't think there's possible future use cases where we'd need this for non-mobile reasons? (maybe if we need to back-populate some activity or something?)
though i do think most likely we won't need this except for mobile

[MatthewPiatetsky]

moved the api to the mobile_api django app

[mikix]

though i do think most likely we won't need this except for mobile

But I think (with my tacky "BFFs forever" T-shirt on), let's say the user profile MFE wanted to start recording user activity for goals (which doesn't make sense, but for the sake of argument). I'd argue that the profile MFE could add its own endpoint in its own BFF djangoapp. Or add a backend call to one of its existing endpoints. Rather than trying to have this API in a general spot that multiple clients could use.

The broader point here is that general purpose APIs, with arbitrary consumers, versioned (but never/rarely incremented) - we are moving away from those in general for this kind of work.

Consider the API Trichotomy: https://openedx.atlassian.net/wiki/spaces/AC/pages/790036554/API+Trichotomy+Proposal

We're building a feature-driven API - we only need to target specific clients and have no need for a support lifecycle implied by a version number, etc.

[MatthewPiatetsky]

ah, that doc is helpful context, good to know!

[mikix]

What is mobile going to do with that? That's a deep in the weeds database ID for a model that we don't have a RESTful API for.

[MatthewPiatetsky]

isn't returning the id of a created object somewhat standard?
i think this would also be helpful for testing their implementation/debugging any issues that could come up
i guess if you think this is an anti-pattern of sorts i could just return a text response?

[mikix]

I do think it's an anti-pattern if you aren't building a RESTful API - and I don't think that's what you're doing here. The client never does anything with this ID value, right? There's no CRUD style "delete this user activity record" or "update this record" calls.

You don't need a text response do you? Just a 200 code?

[MatthewPiatetsky]

sure, i guess i can just return the code and i'll add a log so mobile team can debug

[mikix]

400 is aggressive if you end up moving the waffle down. (Speaking of, the waffle isn't checked in this endpoint)

[MatthewPiatetsky]

i added the waffle mostly as a temporary way for me to test the mobile endpoints from the mobile apps without breaking things in the meantime and was going to delete it once that was done.

I don't know if we need a waffle flag for this long term (or if you think we do, we can probably reuse the goals flag, since the goals feature won't work without this anyways)

[mikix]

No I don't care about the fate of the waffle flag per se. I was just suggesting that it's an aggressive response to a None return from the method - that not recording an item of user activity was necessarily an error state. There could be valid reasons why you wouldn't (waffle is one example, maybe there are others).

But this was more of a nit / prompt to think about whether you want that to be an error state as you refactor things. At least document in the docstring that None means something went wrong - so people on both sides of the function know that's the contract. (Alternatively, if all those cases are actually bad inputs, you could have the method raise exceptions and catch it here - which would also let you send back or log a more specific-to-the-problem message here)

[MatthewPiatetsky]

ah, well currently if the method records activity or finds cached activity, it is returning the object, so it only returns none if the inputs were invalid (or with my modifications now if the user was masquerading)
but given that mobile app doesn't have masquerade i believe and if we're going to make this mobile specific, then it would still be true that this return value would correspond to some sort of invalid inputs
not sure if you think there is a response code that would be better suited to that?

[mikix]

not sure if you think there is a response code that would be better suited to that?

My beef was no so much with the specific response code, but with the design of the function's interface.

A return value of None is fine. But it doesn't distinguish between the benign "nothing needed doing, all good" (waffle case, masquerading, maybe similar future cases, etc) and the client error of "you passed in invalid arguments".

Some solutons:

• Merely document that None is actually an error state and should be treated as such. This method does not have (and apparently promises to never have) "nothing needed doing" benign states. That's the promise that I'm having a hard time with here - why make that? Why leave that gotcha for some future engineer? If you really do leave it like this, at least make heavily document that a None return is a client error and should be treated as such, so an engineer will hopefully know not to add a benign case in the future (though, if they want to, how would they do that? they'd need to refactor the interface along these lines below, yeah? or fake a non-None response)
• Trust that your code in this view that parses the user id and gets it from the db and parses the course-key would already catch the bad states. And just always return 2xx. Removing a need to distinguish between the error and benign cases.
• Treat caller-error cases in a more pythonic (but sometimes more annoying to code around) way by throwing an exception in the record activity function. Then this code could catch it and return a 400 (with the added benefit of a specific error string). This leaves None to signal the benign "nothing needed doing" state.

[MatthewPiatetsky]

I already had documentation around the None return in the method docstring, but its true that i'm already checking the input cases, masquerade isn't a feature in mobile and any other errors would throw an exception anyway, so I've modified the code with your approach from bullet #2

[mikix]

Why if course_tab_list:?

More broadly, why this method? Feels like a very surprising side effect. Was it just a method you found that both courseware and course home called?

I might just have two calls (one for each metadata), rather than having such a surprising side effect for this method. And that would go down to one call anyway when https://openedx.atlassian.net/browse/AA-1018 is fixed.

[MatthewPiatetsky]

doesn't really make sense to record activity if a user can't see any tabs

yes but more importantly this also covers the tabs that are not in the MFE and the courseware that is not in the MFE, since we need to cover those things for this as well as the MFE

[mikix]

doesn't really make sense to record activity if a user can't see any tabs

When would there not be any tabs?

yes but more importantly this also covers the tabs that are not in the MFE and the courseware that is not in the MFE, since we need to cover those things for this as well as the MFE

Interesting. I'm still skeptical that this is a reasonable side effect for this method. I would never in a million years guess that get_course_tab_list also wrote to the database that the user did something that day. Which suggests that it's in the wrong place to me.

If you do keep it here, I think you need to add blaring sirens to the docstring about this. But I would have a think about if there isn't a more natural place to put it.

[MatthewPiatetsky]

if course_tab_list is empty there would be no tabs - presumably not an expected case, but seems technically possible.

i do agree that is confusing, though i think hooking into the backend code for a get request would always be an unexpected side effect

i guess i could do the following:
rather than having one location, i could

1. Create a call alongside the call to this one in course_navigation to cover the non-mfe experience
2. Add a call in the courseware_api
3. Add a call in the course home api
   Not sure if that solves for your concern about modification of the database being an unexpected side effect?
   i don't think i would intuitively expect that calling a course metadata endpoint would populate data on the backend either
   (though we do that as well for other things such as the streak)

[MatthewPiatetsky]

I've updated the code to implement what i described in the above comment

[mikix]

if course_tab_list is empty there would be no tabs - presumably not an expected case, but seems technically possible.

OK... But that never happens right? And even if it did, the user still visited us and we should record that, yeah?

i think hooking into the backend code for a get request would always be an unexpected side effect

I disagree with that. And if you think that, do you view this whole feature as a bad design?

If we're talking a RESTful CRUD style API, then yes - modifying the object under discussion on a GET would certainly be unexpected. But I would still expect a server log file entry to be generated somewhere for that GET. Maybe other analytics recorded too. And this feature is basically analytics.

If we take a step up to a less-CRUD like API (like this) or an html page browser GET, I would absolutely not be surprised at side effects. Maybe creating stuff lazily as the user views pages. Or much more common, lots and lots of analytic records get generated on each view. New Relic, Segment, django, caching, etc.

But it's when we get to the level of a Python method, styled as a code-level GET like get_course_tab_list - that's where side effect database writes/analytics become more surprising to me.

rather than having one location, i could

That approach feels less surprising to me (having an endpoint view directly record the activity).

I'm also open to some other method that those paths all happen to call, if it had a name that could imply what we are doing here. Like, if they happened to all call some method do_analytics which sent a segment event and now this table, and who knows what else. You could make such a method if it doesn't exist, with just this one call in it for now, but that seems maybe too YAGNI.

[mikix]

(didn't see you had already changed it by the time I made my comment)

[mikix]

Oh I hadn't noticed this before - you're taking all arguments as json rather than via the URL like /record_user_activity/course-v1:edx+DemoX+Run/12345 - I think I like your version better, because there isn't an obvious ordering there.

I also dislike that we tend to use underscores rather than hyphens for our endpoint names. A Python-ism that crept into our REST design. But not something you need to change. I'm just talking out loud.

[MatthewPiatetsky]

@mikix i think i've either replied to or addressed all your comments?

[mikix]

Looks good I think! I've added some nits here, but nothing critical. Thanks Matthew!

[mikix]

Why return the id here? It's never used, and feels like an api oddity - is this call trying to abstract the model or not? Are callers ever going to make further edits to the record row?

This method feels aimed at endpoints that doesn't care about goals or user activity rows - they just want a fire and forget one liner to throw in view code. As a nit only -- this doesn't really matter -- this feels like an api.py method more than a model method.

[MatthewPiatetsky]

hm, it is true that i'm not using the id right now, but if i remove it then there's no way to tell if an object got added or not, which seems awkward?

[mikix]

Well, what do callers want/need to know? You currently only use the return value in one place, to respond with either 200 or 201. Though there was a moment where you were using the return value to indicate success/failure, and could conceivably need that again.

If I were writing this, I would probably not bother with any return for this method and have the endpoint always respond with 200. Just because the 201 distinction isn't super meaningful here, I don't think. If I later needed to grow the ability to flag errors again, I'd probably raise exceptions or return a success boolean at that point.

But sticking with what you have here but also trying to avoid returning a database ID... maybe return (success, created)? Or possibly just have created be a tri-state None, True, False?

[MatthewPiatetsky]

hm, i guess the 200 vs 201 doesn't really matter so i've removed the created argument.
i do like the idea of raising an exception instead of returning None, but i don't want to have to add that in the million files if i'm just going to remove those shortly, but i'm just returning None or the id now and maybe once I remove all the different mobile calls and there will only be 3-4 calls left, I can remove the return value altogether and change it to raise an exception (but i think i can leave that in the meantime so there's at least some way to tell if the method failed)

[mikix]

Oy, this is better than being in get_course_tab_list - but another weird little place we're squirreling these away.

[MatthewPiatetsky]

open to other locations if you have suggestions

[mikix]

I still maintain that this is bad API design, to push context like "what are the conditions in which this method should be skipped" onto callers of the API. I'm only harping on this because I think it's a mistake that happens a lot in the platform - where business logic bleeds outside of the abstractions meant to hide it. This is just a nit - I understand that the flag is temporary, so this is not a big deal.

[MatthewPiatetsky]

i do agree but i think once this is tested i can remove this flag and add the goals flag inside the method

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

[edx-pipeline-bot]

EdX Release Notice: This PR has been deployed to the production environment.

[edx-pipeline-bot]

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage