feat: add python apis to agreements models #32967

ericanwoga · 2023-08-09T18:20:29Z

Description

JIRA: https://2u-internal.atlassian.net/browse/MST-2014

In order to support the asynchronous Celery task and the courseware API view, we need to add Python APIs to read from and write to the new agreements models LTIPIITool, LTIPIISignature, and ProctoringPIISignature.

Create a Python API to read from and write to these models. Use the guidance documented in OEP-49: Django App Patterns.

MichaelRoytman

This is a great start!

I know that the existing API methods return model instances or QuerySets, but I think we should avoid doing that here. You can see OEP-49: Django App Patterns for an explanation of why. We don't want to expose model instances or QuerySets. That's why we have this API - so we can encapsulate the models. Instead, we can create a data class that is a simple data container for the data, which can safely be passed outside the agreements application.

I mentioned that it might be helpful to share what information we anticipate the frontend needing from the platform when writing these Python APIs. We'll add the below information to the web API, but that web API will use the Python APIs you're writing here. If you're curious, the web API is in the courseware_api Djangoapp. You can see the user_needs_integrity_signature key in that web API, which corresponds to the IntegritySignature feature in this application. Here are the keys I'd anticipating sending from the that web API.

pii_receiving_lti_tools
This will be a list of dictionaries describing the LTI tools that receive PII in the course. I anticipate the dictionary only having a key for the LTI tool URL or some other identifier, but I think we should make this a dictionary to make it more flexible in case we need to add additional data in the future. I imagine we'll need your read Python API method that reads the list of PII receiving tools here. This will be used by the frontend so that we can display a list of PII receiving LTI tools to the user.
user_needs_lti_pii_signature
This value is a boolean that represents whether the user needs to sign the LTI PII agreement dialog. This will be used to determine whether or not to show the acknowledgment dialog to the user. We'll need a few pieces of information to determine this value.

We'll need...
- whether there are any PII receiving LTI tools in the course
- whether the user already signed an LTI PII acknowledgment
- whether the user's existing LTI PII signature is out-of-date, if one exists; that's determine by comparing the lti_tools_hash of their existing LTI PII Signature and the lti_tools_hash of the current PII receiving tools in the course
Can you think of what sorts of functions you'd need to write to accomplish this? You'll be able to write the code to combine multiple Python API functions to determine this value; you don't need to necessarily write a singular Python API function that determines this.

These changes cover the information the frontend will need from the agreements application to decide how and when to show the agreement to the user.

But we also need to think about recording LTIPIISignatures, right? In that case, the frontend will call our REST API. We'll have to add a REST API POST handler to the agreements application.

What information will that POST handler need to create an LTIPIISignature? We'll probably need the user, the course ID, and the list of PII receiving LTI tools that the user was shown when they signed the acknowledgment, right? Does that match what you'd expect? Do you think we need additional data?

I think you have a good foundation here, but I hope these additional details help you think about what information you'd like to expose via the Python API.

At a minimum, I'd like to see the addition of a data class to those read API functions.

Let me know what you think!

MichaelRoytman · 2023-08-09T20:25:51Z

openedx/core/djangoapps/agreements/api.py

+        * True or False depending on if the write was successful
+    """
+    course_key = CourseKey.from_string(course_id)
+    LTIPIITool.objects.update_or_create(


I think you may have copied and pasted from set_ltipiitool. This should be LTIPIISignature.

MichaelRoytman · 2023-08-09T20:37:04Z

openedx/core/djangoapps/agreements/api.py

@@ -68,3 +70,76 @@ def get_integrity_signatures_for_course(course_id):
    """
    course_key = CourseKey.from_string(course_id)
    return IntegritySignature.objects.filter(course_key=course_key)
+
+
+def get_ltipiitool(course_id):


I'd like to see a function name that is less tied to the underlying model name. Maybe something like get_pii_receiving_lti_tools? Or whatever you think is best. I know it's virtually the same, but the current functions name reads like it's returning a model instance - because it is - and I don't think it should return a model instance - please see my overall pull request comment.

MichaelRoytman · 2023-08-09T20:37:26Z

openedx/core/djangoapps/agreements/api.py

+        return None
+
+
+def set_ltipiisignature(course_id, lti_tools, lti_tools_hash):


When a learner signs the acknowledgment, we'll make a POST from the frontend to the agreements REST API on the platform. That POST handler will call into this Python API to record the signature. Where in that flow do you think it would make sense to compute the lti_tools_hash?

I was thinking it would make sense to do here. If that's the case, I think this function should take course_id and lti_tools as parameters and computer the LTI hash within the method instead of letting the caller of the REST API or the REST API itself compute it. What do you think?

chennighan2u · 2023-08-28T18:20:59Z

openedx/core/djangoapps/agreements/views.py

+        username = request.user.username
+        signature = create_lti_pii_signature(username, course_id, lti_tools)
+        #serializer = IntegritySignatureSerializer(signature)
+        #return Response(serializer.data)


What's the future for these commented out bits?

I think they will be replaced with a serializer specifically for the LTIPIISignature

Since this isn't ready yet and will be added a later point, what do you think about removing this from this pull request?

MichaelRoytman

This is looking good! I think there are a few tweaks you could make before closing this out, which I left as comments, but this is getting close. Let me know what you think about my comments. Thanks!

MichaelRoytman · 2023-09-12T20:39:37Z

openedx/core/djangoapps/agreements/api.py

+    """
+
+    course_key = CourseKey.from_string(course_id)
+    course_ltipiitools = LTIPIITool.objects.get(course_key=course_key).lti_tools


You will need to add the same error handling for the LTIPIITool.DoesNotExist exception as you have in the get_lti_pii_signature function here. Otherwise, if a course has no LTI tools and no row in the LTIPIITool table, this call with raise an exception.

MichaelRoytman · 2023-09-12T20:40:03Z

openedx/core/djangoapps/agreements/api.py

+        * False if the course does not.
+    """
+    course_key = CourseKey.from_string(course_id)
+    course_lti_pii_tools = LTIPIITool.objects.get(course_key)


You will need to add the same error handling for the LTIPIITool.DoesNotExist exception as you have in the get_lti_pii_signature function here. Otherwise, if a course has no LTI tools and no row in the LTIPIITool table, this call with raise an exception.

MichaelRoytman · 2023-09-12T21:14:31Z

openedx/core/djangoapps/agreements/api.py

+    user = User.objects.get(username=username)
+    course_key = CourseKey.from_string(course_id)
+
+    signature = LTIPIISignature.objects.get(user=user, course_key=course_key)


You'll want the error handling here too.

MichaelRoytman · 2023-09-12T21:14:36Z

openedx/core/djangoapps/agreements/api.py

+    user = User.objects.get(username=username)
+    course_key = CourseKey.from_string(course_id)
+
+    user_lti_pii_signature_hash = LTIPIISignature.objects.get(course_key=course_key, user=user).lti_tools_hash


You'll want the error handling here too.

Would the try-except clause include the user.objects.get() line as well? I am thinking that if a user is in the course they'd have to exist, but I'm not sure if it's safe to assume.

That's a good question. You have a good point that we'd really only expect for these APIs to be called in the context of someone being logged in and navigating a course, in which case they would definitely exist in the database and this call would always succeed.

But, to be safe, I think it would be best to wrap this call in a try-except block as well on the off chance that these APIs are used inappropriately without a valid username. Stuff happens!

MichaelRoytman · 2023-09-12T21:15:13Z

openedx/core/djangoapps/agreements/api.py

+    course_key = CourseKey.from_string(course_id)
+    course_lti_pii_tools = LTIPIITool.objects.get(course_key)
+
+    if not course_lti_pii_tools:


What if there is no entry in the database for this course (i.e. no QuerySet and exception is raised)?

MichaelRoytman · 2023-09-12T21:43:15Z

openedx/core/djangoapps/agreements/api.py

+    course_lti_pii_tools_hash = LTIPIITool.objects.get(course_key=course_key).lti_tools_hash
+
+    if user_lti_pii_signature_hash == course_lti_pii_tools_hash:
+        # Hashes are equal, therefor update is not need


Nit.

Suggested change

# Hashes are equal, therefor update is not need

# Hashes are equal, therefore update is not need

MichaelRoytman · 2023-09-12T21:50:59Z

openedx/core/djangoapps/agreements/data.py

+    """
+    Class that stores an lti pii signature
+    """
+    lti_pii_signature: LTIPIISignature


This is still exposing the data model, albeit indirectly. Callers that receive an instance of LTIPIISignatureData will still be able to access the LTIPIISignature instance through the lti_pii_signature attribute. We want to try to use Python primitives here or other classes or constructs that are not the model instance itself. The idea is we want to avoid anyone from outside the application (i.e. anyone using the Python API) from getting an instance of the LTIPIISignature model class.

You want to be able to give the consumer a container of data they need - a representation of the data in the database - without handing them a representation of the way the data is stored in the database or a representation that allows them to interact with the model instance and call its methods and inspect its properties.

For example, you could do the following...

@attr.s(frozen=True, auto_attribs=True) class LTIPIISignatureData: """ Class that stores an lti pii signature """ user_id: str course_id: str lti_tools: str lti_tools_hash: str

What do you think of that?

Thank you for catching this. Not sure why I did that.

MichaelRoytman · 2023-09-12T21:52:35Z

openedx/core/djangoapps/agreements/tests/test_api.py

+        """
+        Test that duplicate lti pii signatures cannot be created
+        """
+        print(self.user, self.course, self.course_id, self.lti_tools)


Before you merge, please remove this print statement.

MichaelRoytman · 2023-09-12T21:55:18Z

openedx/core/djangoapps/agreements/tests/test_api.py

+        print(self.user, self.course, self.course_id, self.lti_tools)
+        with LogCapture(LOGGER_NAME, level=logging.WARNING) as logger:
+            create_lti_pii_signature(self.user.username, self.course_id, self.lti_tools)
+            create_lti_pii_signature(self.user.username, self.course_id, self.lti_tools)


This is a nice test! I think you could improve it by changing the lti_tools in the second call to create_lti_pii_signature. Right now, you don't actually know whether the code re-created the LTIPIISignature, because the data in the first call and the second call are identical. data.lti_pii_signature could be representing the first call or the second - they're the same. If you make them different, calling _assert_lti_pii_signature will tell you absolutely that only the first LTIPIISignature was created.

Ah! It seems like by changing the tools of the second call, it stores both signatures.

Now, I'm thinking that using LTIPIISignature.update_or_create() would be more suitable than LTIPIISignature.get_or_create(). That way, we won't need separate functions to handle updating existing signatures.

MichaelRoytman

This is looking good! I left some more comments.

MichaelRoytman · 2023-09-22T15:29:34Z

openedx/core/djangoapps/agreements/api.py

+    course_key = CourseKey.from_string(course_id)
+    try:
+        course_ltipiitools = LTIPIITool.objects.get(course_key=course_key).lti_tools
+        return LTIToolsReceivingPIIData(


I think it would be better to move the return statement outside of the try block, since we don't expect any ObjectDoesNotExist exception to be raised here. Keeping just the code that raises the exception we're handling in the except clause in the try block makes it easier to reason about the code.

MichaelRoytman · 2023-09-22T15:30:05Z

openedx/core/djangoapps/agreements/api.py

+            course_key=course_key,
+            lti_tools=lti_tools,
+            lti_tools_hash=lti_tools_hash)
+        return signature


Nit. Since both the except and else clauses return signature, you could move the return statement outside of the try...except...else block.

MichaelRoytman · 2023-09-22T15:32:55Z

openedx/core/djangoapps/agreements/api.py

+        if _user_lti_pii_signature_exists(username, course_id):
+            if _user_needs_signature_update(username, course_id):
+                # up to date
+                return False


I think this should be True and the else clause should be False, right? The _user_needs_signature_update function returns whether the user needs an updated signature, so this should be returning True to represent that the user needs to sign the agreement again.

Yes that's right! Thank you fir catching that

MichaelRoytman · 2023-09-22T15:42:33Z

openedx/core/djangoapps/agreements/api.py

+        * True if the user needs to sign a new acknowledgement.
+        * False if the acknowledgements are up to date.
+    """
+    if _course_has_lti_pii_tools(course_id):


This looks good and captures most of the complex logic of determining whether a user needs an LTI PII signature - suggested a small correction below. Nice job!

I think we could simplify this a little bit. I don't think we need three nested if statements. We can accomplish this without that. You'll notice that there are a few duplicated return statements.

To figure out how to write this more succinctly, I created a matrix of the boolean values of has_lti_pii_tools, lti_pii_signature_exists, and lti_pii_signature_needs_update and how they they all resolve to the value of user_needs_lti_pii_signature.

That led me to write something like this...

course_has_lti_pii_tools = _course_has_lti_pii_tools(course_id) user_has_signature = _user_lti_pii_signature_exists(username, course_id) signature_needs_update = _user_needs_signature_update(username, course_id) if course_has_lti_pii_tools && (not user_has_signature || signature_needs_update): return True else: return False

What do you think of that? You could try creating your own matrix of booleans and see if this does the same thing your code does.

I'll try out this approach!

MichaelRoytman · 2023-09-22T15:50:10Z

openedx/core/djangoapps/agreements/api.py

+
+    # if user and course exists, update, otherwise create a new signature
+    try:
+        LTIPIISignature.objects.get(user=user, course_key=course_key)


Using the ObjectDoesNotExist exception in a try...catch block will "will catch DoesNotExist exceptions for all models". This could be a problem because we might catch exceptions for other models that we don't expect, and then the exception is buried and may not be handled appropriately. I think it would be better to catch the exception that you anticipate and want to handle, which is LTIPIISignature.DoesNotExist. You can see the documentation for this exception here.

I see this ObjectDoesNotExist class in a few places in this file.

Ohh, I was wondering how to throw a DoesNotExist exception for specific models. I just resorted to ObjectDoesNotExist. I will correct the rest of them as well.

MichaelRoytman · 2023-09-22T15:53:57Z

openedx/core/djangoapps/agreements/api.py

+    """
+    course_key = CourseKey.from_string(course_id)
+    try:
+        LTIPIITool.objects.get(course_key)


Suggested change

LTIPIITool.objects.get(course_key)

LTIPIITool.objects.get(course_key=course_key)

Does this work? I think you need to specify the model field against which you're doing the match on course_key. Or does this work because course_key is the primary key?

Nope, this was a mistype on my end.

MichaelRoytman · 2023-09-22T15:54:51Z

openedx/core/djangoapps/agreements/api.py

+        user = User.objects.get(username=username)
+        LTIPIISignature.objects.get(user=user, course_key=course_key)
+    except ObjectDoesNotExist:
+        return None


Suggested change

return None

return False

This cases is when a signature doesn't exist, so would False be more appropriate here than None?

Yep, it should be False, since the function returns a boolean value.

MichaelRoytman · 2023-09-22T15:55:44Z

openedx/core/djangoapps/agreements/api.py

+        user = User.objects.get(username=username)
+        user_lti_pii_signature_hash = LTIPIISignature.objects.get(course_key=course_key, user=user).lti_tools_hash
+        course_lti_pii_tools_hash = LTIPIITool.objects.get(course_key=course_key).lti_tools_hash
+    except ObjectDoesNotExist:


You can specify a set of exceptions to handle here, like User.DoesNotExist, LTIPIISignature.DoesNotExist, and LTIPIITool.DoesNotExist instead of the generic ObjectDoesNotExist exception; see my comment above.

MichaelRoytman · 2023-09-22T15:56:08Z

openedx/core/djangoapps/agreements/api.py

+        user_lti_pii_signature_hash = LTIPIISignature.objects.get(course_key=course_key, user=user).lti_tools_hash
+        course_lti_pii_tools_hash = LTIPIITool.objects.get(course_key=course_key).lti_tools_hash
+    except ObjectDoesNotExist:
+        return None


Similar to above, do you think this should be False? Or None?

Yep, it should be False.

Also, would the ObjectDoesNotExist exception be okay since any exception caught should return False? Or is it better practice to have separate cases for User.DoesNotExist, LTIPIITool.DoesNotExist, and LTIPIISignature.DoesNotExist?

That's a good question. I lean towards being more explicit over implicit. Those are the three exceptions we know are possible and expect to be raised in certain conditions. If we just do ObjectDoesNotExist, it's implied that we're handling those three exceptions, but it's not explicit because we haven't listed what we're handling. The except clause will catch any <model>.DoesNotExist exception. That means that if another exception is raised that we don't expect for another model, it'll get silently handled by this exception handler and we'll never know about it. And it may even incorrectly handle the exception.

I guess it's pretty low risk here since's your explicitly calling the model API, so it would be bizarre to get any other exception, but I still think it's better practice.

MichaelRoytman · 2023-09-22T15:57:07Z

openedx/core/djangoapps/agreements/data.py

+    """
+    Class that stores data about the list of LTI tools sharing PII
+    """
+    lii_tools_receiving_pii: {}


Once we get to the modulestore stage of the project, we may want to change this a bit to give more expected structure to each value in the dictionary, but I think this is fine as it is currently.

MichaelRoytman

This looks great! There are just one really small thing I would like you to fix. The rest are at your discretion - you can take or leave whatever you like. I think this will be ready to merge really soon! 🎉

There are a few failing linting checks you should take a look at as well - these are the test failures on your pull request.

MichaelRoytman · 2023-09-28T20:22:28Z

openedx/core/djangoapps/agreements/api.py

+    except LTIPIITool.DoesNotExist:
+        return None
+
+    return LTIToolsReceivingPIIData(lii_tools_receiving_pii=course_ltipiitools,)


Nit.

Suggested change

return LTIToolsReceivingPIIData(lii_tools_receiving_pii=course_ltipiitools,)

return LTIToolsReceivingPIIData(lii_tools_receiving_pii=course_ltipiitools)

MichaelRoytman · 2023-09-28T20:23:27Z

openedx/core/djangoapps/agreements/api.py

+        # write a new signature, or update an existing signature
+        return True
+    else:
+        # course does not have lti pii tools, or signatue is up to date


Nit.

Suggested change

# course does not have lti pii tools, or signatue is up to date

# course does not have lti pii tools, or signature is up to date

MichaelRoytman · 2023-09-28T20:29:40Z

openedx/core/djangoapps/agreements/api.py

+        # no entry in the database
+        return False
+    else:
+        return True


The only potential issue here is in the case that a course is in the database with no tools. This could happen if a course has tools that receive PII and then those tools are configured not to share PII. Because we don't yet know how we're going to handle that (i.e. remove that course's row from the database or just have the list of tools be empty and keep the row), I think it would be safest to check for both the LTIPIITool.DoesNotExist exception and check whether the LTIPIITool.lti_tools is empty. Does that make sense?

MichaelRoytman · 2023-09-28T20:32:09Z

openedx/core/djangoapps/agreements/api.py

+    except (User.DoesNotExist, LTIPIISignature.DoesNotExist, LTIPIITool.DoesNotExist):
+        return False
+    else:
+        if user_lti_pii_signature_hash == course_lti_pii_tools_hash:


Nit. You can shorten this a bit, if you want. Because a==b evaluates to a boolean itself, you can just return the result of your logical expression. For example, return user_lti_pii_signature_hash != course_lti_pii_tools_hash will accomplish the exact same thing and is a little easier to read.

MichaelRoytman · 2023-09-28T20:33:41Z

openedx/core/djangoapps/agreements/views.py

+        username = request.user.username
+        signature = create_lti_pii_signature(username, course_id, lti_tools)
+        #serializer = IntegritySignatureSerializer(signature)
+        #return Response(serializer.data)


Since this isn't ready yet and will be added a later point, what do you think about removing this from this pull request?

MichaelRoytman

Thank you for all your hard work getting this done! 🎉

edx-pipeline-bot · 2023-10-04T15:43:43Z

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

edx-pipeline-bot · 2023-10-04T16:02:52Z

2U Release Notice: This PR has been deployed to the edX production environment.

MichaelRoytman requested changes Aug 9, 2023

View reviewed changes

chennighan2u reviewed Aug 28, 2023

View reviewed changes

ericanwoga force-pushed the enwoga/apis-for-agreements-models branch from ba0a268 to 56be369 Compare September 11, 2023 18:28

MichaelRoytman requested changes Sep 13, 2023

View reviewed changes

ericanwoga added 7 commits September 20, 2023 12:47

feat: added python apis to agreements models

2c1a596

feat: add python apis to agreements app

ed5a1e2

feat: updated pyton apis and rest api

98ef1e9

feat: fixed python apis and added tests

add1fbe

test: Added error handling and updated test functions

7715f23

style: fixed test formatting

03adda8

style: fixed test formatting

ac5e15f

ericanwoga force-pushed the enwoga/apis-for-agreements-models branch from 23a20af to ac5e15f Compare September 20, 2023 16:48

style: fixed test formatting

421e1b5

MichaelRoytman reviewed Sep 22, 2023

View reviewed changes

ericanwoga added 2 commits September 27, 2023 13:34

feat: Fixed error handling for python APIs

81afd41

feat: Fixed error handling for python APIs

92ad9b3

MichaelRoytman reviewed Sep 28, 2023

View reviewed changes

ericanwoga added 2 commits September 29, 2023 14:22

fix: edited python api for agreements app

0c784f6

style: quality revisions for python apis

bf846f5

MichaelRoytman approved these changes Oct 3, 2023

View reviewed changes

ericanwoga merged commit 3d26512 into master Oct 4, 2023
61 checks passed

ericanwoga deleted the enwoga/apis-for-agreements-models branch October 4, 2023 14:36

		return None


		def set_ltipiisignature(course_id, lti_tools, lti_tools_hash):

	# Hashes are equal, therefor update is not need
	# Hashes are equal, therefore update is not need

	LTIPIITool.objects.get(course_key)
	LTIPIITool.objects.get(course_key=course_key)

	return LTIToolsReceivingPIIData(lii_tools_receiving_pii=course_ltipiitools,)
	return LTIToolsReceivingPIIData(lii_tools_receiving_pii=course_ltipiitools)

	# course does not have lti pii tools, or signatue is up to date
	# course does not have lti pii tools, or signature is up to date

feat: add python apis to agreements models #32967

feat: add python apis to agreements models #32967

Conversation

ericanwoga commented Aug 9, 2023

Description

MichaelRoytman 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

MichaelRoytman 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

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

ericanwoga Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

MichaelRoytman 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

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

MichaelRoytman 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

MichaelRoytman left a comment

Choose a reason for hiding this comment

edx-pipeline-bot commented Oct 4, 2023

edx-pipeline-bot commented Oct 4, 2023

ericanwoga Sep 15, 2023 •

edited

Loading