change to new edx user service #224

sroertgen · 2022-01-25T15:15:21Z

As discussed in #217 edX changed its internal user service.

This PR implements these changes.

openedx-webhooks · 2022-01-25T15:15:28Z

Thanks for the pull request, @sroertgen! I've created OSPR-6413 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

giovannicimolin · 2022-01-26T14:17:38Z

@sroertgen Awesome work here.
Before we can accept code, we need a signed contributor agreement. Can you follow the instructions above?

Let me know if you have any questions. I'll find the right person to help you if you bump into any issues.

natabene · 2022-01-27T17:49:51Z

@sroertgen
Thank you for the contribution! Once you have signed our Contributor Agreement and it has been processed, you will receive a confirmation via email from our Legal team. We then will be able to review this and all your future code contributions.

sroertgen · 2022-01-28T07:35:35Z

Thank you both!
I filled out the document and send it to your legal team.

Have a nice weekend!

natabene · 2022-01-28T15:19:03Z

@sroertgen Thank you very much! Once they process it and you are cleared, I will post here.

giovannicimolin · 2022-01-31T19:57:03Z

@sroertgen Some tests are failing because of the User service mock isn't returning proper values, can you fix that? See https://github.com/openedx/xblock-lti-consumer/runs/4982515609?check_suite_focus=true.

Edit: I noticed that commitlint in failing here as well.

You'll need to:

Bump the version on https://github.com/openedx/xblock-lti-consumer/blob/master/lti_consumer/__init__.py#L7
Reword your commits to follow OEP-51. Just squash and word your commit like this: fix: Replace deprecated runtime methods with user service methods.

Thanks for your patience here, the strict CI/rulechecks can be a hassle if it's your first time around. Let me know if you need any help. 😁

sroertgen · 2022-02-02T14:17:48Z

@giovannicimolin oh yes, there are tests :D

I will have a look into this! And thanks for the hints!

natabene · 2022-02-04T03:27:38Z

@sroertgen Thank you for signing, you are all set now. Next commit or rebase should clear the cla check.

sroertgen · 2022-02-04T06:43:34Z

Hey @giovannicimolin,

I fixed the tests. Had a bit of struggle squashing my commits, but I hope it is now as you expect (was a good opportunity for me starting with this best practice ;) )

Please let me know, if I have to do anything else.

sroertgen · 2022-02-04T12:18:59Z

just saw the failing test. I just ran make test, but there seem to be more tests as it seems 😃
Will have a look into it

giovannicimolin · 2022-02-04T19:01:02Z

@sroertgen There's quality checks too, make sure you run those as well. :)

fix failing quality tests

sroertgen · 2022-02-09T18:48:41Z

@giovannicimolin If you don't mind, you can try running the workflows again.

giovannicimolin · 2022-02-09T20:07:01Z

@sroertgen Tests running 😁

codecov · 2022-02-09T20:10:41Z

Codecov Report

Merging #224 (8c74c7d) into master (6491871) will decrease coverage by 0.04%.
The diff coverage is 95.74%.

@@            Coverage Diff             @@
##           master     #224      +/-   ##
==========================================
- Coverage   97.14%   97.10%   -0.05%     
==========================================
  Files          66       66              
  Lines        4694     4698       +4     
==========================================
+ Hits         4560     4562       +2     
- Misses        134      136       +2

Flag	Coverage Δ
unittests	`97.10% <95.74%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
lti_consumer/lti_xblock.py	`95.09% <81.81%> (-0.51%)`	⬇️
lti_consumer/tests/unit/test_lti_xblock.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6491871...8c74c7d. Read the comment docs.

natabene · 2022-02-11T18:07:33Z

@sroertgen @giovannicimolin Is this good for our review?

giovannicimolin

@sroertgen Did a full review and testing of the changes and left a few suggestions in the PR.

lti_consumer/lti_xblock.py

giovannicimolin · 2022-02-16T13:44:54Z

lti_consumer/lti_xblock.py

+        try:
+            role = self.runtime.service(self, 'user').get_current_user().opt_attrs['edx-platform.user_role']
+        # opt_attrs might not be present or the "edx-platform.user_role" key if opt_attrs is present
+        except (KeyError, AttributeError):
+            role = "student"
+        return ROLE_MAP.get(role, 'Student')


You can always consider opt_attrs as a valid attribute (this is defined here), simplifying this to:

Suggested change

try:

role = self.runtime.service(self, 'user').get_current_user().opt_attrs['edx-platform.user_role']

# opt_attrs might not be present or the "edx-platform.user_role" key if opt_attrs is present

except (KeyError, AttributeError):

role = "student"

return ROLE_MAP.get(role, 'Student')

role = self.runtime.service(self, 'user').get_current_user().opt_attrs.get('edx-platform.user_role', 'student')

return ROLE_MAP.get(role, 'Student')

lti_consumer/lti_xblock.py

sroertgen · 2022-02-21T08:56:26Z

Thanks! Will have a look into this during this week!

…umer

sroertgen · 2022-03-09T16:09:04Z

Hey @giovannicimolin,

sorry it took a bit longer, but I had to hand in two proposals in the last weeks and just did not have time to work on your suggestions.

Thanks for the hints! I implemented the changes and am looking forward for your feedback.

Best,
Steffen

sroertgen · 2022-03-10T08:03:46Z

Sorry, my make quality test suggested to delete the line that now threw the error. I just put it back in.

natabene · 2022-03-14T14:44:52Z

@giovannicimolin Could you finalize reviewing and merge when ready? Owning team is fine with you review and mege.

giovannicimolin · 2022-03-16T14:19:27Z

@natabene Sure! I'm planning to review this today - sorry for the delay in the reply.

natabene · 2022-03-18T18:17:51Z

@giovannicimolin Thanks!

giovannicimolin

👍

I tested this:

Tested LTI 1.1 launches from both the LMS and Studio and checked the data.
Individually tested each method modified and checked it's results on the LMS and Studio environments using a debugger.

I read through the code
~~I checked for accessibility issues~~ NA
~~Includes documentation~~ NA

@sroertgen Great work! Thanks for the contribution.

I'll fix the commit-lint error using GitHub's squash and merge feature.

openedx-webhooks · 2022-03-18T20:38:21Z

@sroertgen 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added community manager review open-source-contribution PR author is not from Axim or 2U labels Jan 25, 2022

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Feb 4, 2022

sroertgen force-pushed the master branch from 491ad7e to 0ce23d9 Compare February 4, 2022 06:34

fix: Replace deprecated runtime methods with user service methods

451b05e

fix failing quality tests

sroertgen force-pushed the master branch from 0ce23d9 to 451b05e Compare February 7, 2022 10:33

openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 11, 2022

Merge branch 'master' into master

b953e30

giovannicimolin suggested changes Feb 16, 2022

View reviewed changes

Merge branch 'master' of https://github.com/sroertgen/xblock-lti-cons…

a37bf8c

…umer

fix quality test

ce5f769

openedx-webhooks added engineering review and removed awaiting prioritization labels Mar 14, 2022

Merge branch 'master' into master

8c74c7d

giovannicimolin approved these changes Mar 18, 2022

View reviewed changes

giovannicimolin merged commit fd717e0 into openedx:master Mar 18, 2022

openedx-webhooks added merged and removed engineering review labels Mar 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change to new edx user service #224

change to new edx user service #224

sroertgen commented Jan 25, 2022

openedx-webhooks commented Jan 25, 2022 •

edited

Loading

giovannicimolin commented Jan 26, 2022

natabene commented Jan 27, 2022

sroertgen commented Jan 28, 2022

natabene commented Jan 28, 2022

giovannicimolin commented Jan 31, 2022 •

edited

Loading

sroertgen commented Feb 2, 2022

natabene commented Feb 4, 2022

sroertgen commented Feb 4, 2022

sroertgen commented Feb 4, 2022

giovannicimolin commented Feb 4, 2022

sroertgen commented Feb 9, 2022

giovannicimolin commented Feb 9, 2022

codecov bot commented Feb 9, 2022 •

edited

Loading

natabene commented Feb 11, 2022

giovannicimolin left a comment

giovannicimolin Feb 16, 2022

sroertgen commented Feb 21, 2022

sroertgen commented Mar 9, 2022

sroertgen commented Mar 10, 2022

natabene commented Mar 14, 2022

giovannicimolin commented Mar 16, 2022

natabene commented Mar 18, 2022

giovannicimolin left a comment

openedx-webhooks commented Mar 18, 2022

change to new edx user service #224

change to new edx user service #224

Conversation

sroertgen commented Jan 25, 2022

openedx-webhooks commented Jan 25, 2022 • edited Loading

giovannicimolin commented Jan 26, 2022

natabene commented Jan 27, 2022

sroertgen commented Jan 28, 2022

natabene commented Jan 28, 2022

giovannicimolin commented Jan 31, 2022 • edited Loading

sroertgen commented Feb 2, 2022

natabene commented Feb 4, 2022

sroertgen commented Feb 4, 2022

sroertgen commented Feb 4, 2022

giovannicimolin commented Feb 4, 2022

sroertgen commented Feb 9, 2022

giovannicimolin commented Feb 9, 2022

codecov bot commented Feb 9, 2022 • edited Loading

Codecov Report

natabene commented Feb 11, 2022

giovannicimolin left a comment

Choose a reason for hiding this comment

giovannicimolin Feb 16, 2022

Choose a reason for hiding this comment

sroertgen commented Feb 21, 2022

sroertgen commented Mar 9, 2022

sroertgen commented Mar 10, 2022

natabene commented Mar 14, 2022

giovannicimolin commented Mar 16, 2022

natabene commented Mar 18, 2022

giovannicimolin left a comment

Choose a reason for hiding this comment

openedx-webhooks commented Mar 18, 2022

openedx-webhooks commented Jan 25, 2022 •

edited

Loading

giovannicimolin commented Jan 31, 2022 •

edited

Loading

codecov bot commented Feb 9, 2022 •

edited

Loading