feat: add management command for course prompts #28

alangsto · 2023-10-18T14:31:42Z

MST-2164

This PR adds a management command that generates a new set of course prompts given a list of course ids.

alangsto · 2023-10-18T14:32:38Z

learning_assistant/management/commands/set_course_prompts.py

+        parser.add_argument(
+            '--pre_message',
+            dest='pre_message',
+            help='message to prepend to course topics',
+        )
+
+        parser.add_argument(
+            '--skills_descriptor',
+            dest='skills_descriptor',
+            help='message that describes skill structure'
+        )
+
+        # post-message
+        parser.add_argument(
+            '--post_message',
+            dest='post_message',
+            help='message to append to course topics',
+        )


Instead of having three separate args for the messages, I'm wondering if it would be possible to use string formatting instead. This way, one message block could be passed in, and then the topics could be added to that string.

I think either is fine. The string template seems fine, but we should leave a detailed explanation about the desired format.

Opted to leave this as is for now, but left a note describing the required structure.

alangsto · 2023-10-18T14:33:20Z

learning_assistant/management/commands/set_course_prompts.py

+            skills_message = skills_descriptor + json.dumps(course_dict)
+
+            # finally, create list of prompt messages and save
+            prompt_messages = [pre_message, skills_message, post_message]


I haven't tested if this format of message actually saves. When we were manually adding these, it took a bit of wrangling to get the json format correct.

Is this something you can verify when manually testing?

Yes, trying to do that now!

Json formatting looks great, tested locally to confirm

alangsto · 2023-10-18T15:00:26Z

learning_assistant/management/commands/set_course_prompts.py

+        )
+
+    @staticmethod
+    def _get_discovery_api_client(username):


coverage is down because this function isn't being tested - I'm not really sure it's worth mocking everything in here.

I don't feel like we need to do that.

MichaelRoytman · 2023-10-18T15:15:31Z

learning_assistant/management/commands/set_course_prompts.py

+        """
+        Returns an API client which can be used to make Catalog API requests.
+        """
+        User = get_user_model()


Is there a reason not to use the OAuthAPIClient with the BACKEND_SERVICE_EDX_OAUTH2_KEY and BACKEND_SERVICE_EDX_OAUTH2_SECRET settings instead of impersonating a user?

Is this a pattern we have elsewhere? Because it strikes me as a unexpected to use an existing account to make calls to the LMS.

I did find it elsewhere, this is ripped from the catalog app in edx-platform: https://github.com/openedx/edx-platform/blob/92731be0dc69d82cc4ec5b9c511e86df9aa9e632/openedx/core/djangoapps/catalog/utils.py#L51-L60.

Interesting. Those changes eventually lead back to openedx/public-engineering#37, which says, "We will be switching usages of the old client to OAuthAPIClient as part of this removal."

Ed Zeracor left a comment, "In some cases the request is on the behalf of the user and we are passing JOTs around. We will still need to do this, but the current client doesn’t have an interface for supporting JOTs", which I think was the case for the catalog service. I imagine that's why it was implemented this way.

I'd lean toward using the OAuthAPIClient, since it seems like the intended direction. It sounds like to me that the catalog service was implemented this way because the existing code wasn't well supported by OAuthAPIClient, but this is new code. Registrar has an example of calling discovery with OAuthAPIClient - it doesn't look any more complex.

What do you think?

Ohh interesting find. I can try and look into using the OAuthAPIClient, I wasn't finding the credentials I needed so I wasn't sure if I would need to create them in discovery admin.

Updated to use the OauthAPIClient. Opting to use the discovery-worker credentials.

MichaelRoytman · 2023-10-18T15:18:19Z

learning_assistant/management/commands/set_course_prompts.py

+            course_key = CourseKey.from_string(course_run_id)
+
+            # discovery API requires course keys, not course run keys
+            course_id = f'{course_key.org}+{course_key.course}'


I know you've run a query to get the list of courses that have skills. Do any of them use the old style course IDs (e.g. edX/DemoX.1/2014)? If so, will those calls to Discovery fail because you're constructing a course key in the new style?

None of them have the old style of course ID! I do see that as an area for improvement, but I think we are safe for now.

Nice! Thank you. What do you think about leaving a comment that this only supports new style keys - seems like something one could overlook in the future.

MichaelRoytman · 2023-10-18T15:23:39Z

learning_assistant/management/commands/set_course_prompts.py

+        parser.add_argument(
+            '--pre_message',
+            dest='pre_message',
+            help='message to prepend to course topics',
+        )
+
+        parser.add_argument(
+            '--skills_descriptor',
+            dest='skills_descriptor',
+            help='message that describes skill structure'
+        )
+
+        # post-message
+        parser.add_argument(
+            '--post_message',
+            dest='post_message',
+            help='message to append to course topics',
+        )


I think either is fine. The string template seems fine, but we should leave a detailed explanation about the desired format.

MichaelRoytman · 2023-10-18T15:24:06Z

learning_assistant/management/commands/set_course_prompts.py

+        )
+
+    @staticmethod
+    def _get_discovery_api_client(username):


I don't feel like we need to do that.

MichaelRoytman · 2023-10-18T15:30:00Z

learning_assistant/management/commands/set_course_prompts.py

+            skills_message = skills_descriptor + json.dumps(course_dict)
+
+            # finally, create list of prompt messages and save
+            prompt_messages = [pre_message, skills_message, post_message]


Is this something you can verify when manually testing?

varshamenon4

Looks great! Just had one clarifying question and can then approve! :)

learning_assistant/management/commands/set_course_prompts.py

varshamenon4 · 2023-10-19T19:32:59Z

learning_assistant/management/commands/set_course_prompts.py

+            skills_message = skills_descriptor + json.dumps(course_dict)
+
+            # finally, create list of prompt messages and save
+            prompt_messages = [pre_message, skills_message, post_message]


varshamenon4

lgtm!

alangsto commented Oct 18, 2023

View reviewed changes

alangsto force-pushed the alangsto/add_management_command branch 4 times, most recently from 1fdb76a to 9253edc Compare October 18, 2023 14:48

alangsto commented Oct 18, 2023

View reviewed changes

MichaelRoytman reviewed Oct 18, 2023

View reviewed changes

alangsto force-pushed the alangsto/add_management_command branch 5 times, most recently from 70b9094 to 26b511a Compare October 19, 2023 16:13

varshamenon4 reviewed Oct 19, 2023

View reviewed changes

alangsto force-pushed the alangsto/add_management_command branch from 26b511a to d84dbc3 Compare October 20, 2023 14:26

feat: add management command for course prompts

2d8d29d

alangsto force-pushed the alangsto/add_management_command branch from d84dbc3 to 2d8d29d Compare October 20, 2023 14:28

varshamenon4 approved these changes Oct 20, 2023

View reviewed changes

alangsto merged commit 5224f66 into main Oct 20, 2023
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add management command for course prompts #28

feat: add management command for course prompts #28

alangsto commented Oct 18, 2023 •

edited

Loading

alangsto Oct 18, 2023

MichaelRoytman Oct 18, 2023

alangsto Oct 19, 2023

alangsto Oct 18, 2023

MichaelRoytman Oct 18, 2023

alangsto Oct 18, 2023

alangsto Oct 19, 2023

varshamenon4 Oct 19, 2023

alangsto Oct 18, 2023

MichaelRoytman Oct 18, 2023

MichaelRoytman Oct 18, 2023

alangsto Oct 18, 2023

MichaelRoytman Oct 18, 2023

alangsto Oct 18, 2023

alangsto Oct 19, 2023

MichaelRoytman Oct 18, 2023

alangsto Oct 18, 2023

MichaelRoytman Oct 18, 2023

MichaelRoytman Oct 18, 2023

MichaelRoytman Oct 18, 2023

MichaelRoytman Oct 18, 2023

varshamenon4 left a comment

varshamenon4 Oct 19, 2023

varshamenon4 left a comment

feat: add management command for course prompts #28

feat: add management command for course prompts #28

Conversation

alangsto commented Oct 18, 2023 • 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

varshamenon4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varshamenon4 left a comment

Choose a reason for hiding this comment

alangsto commented Oct 18, 2023 •

edited

Loading