Refactor stub implementation of LTI Provider. BLD-601. #2029

olmar · 2013-12-23T15:11:28Z

Refactor stub implementation of LTI Provider.

Issue: https://edx-wiki.atlassian.net/browse/BLD-601.

@auraz please review.
@wedaly please review.

singingwolfboy · 2013-12-23T21:07:18Z

lms/djangoapps/courseware/features/lti_setup.py

Don't commit commented-out code to the repository.

olmar · 2013-12-30T11:04:04Z

@wedaly I`m looking for how it possible to launch such stubs manually. Do you know smth about it?

wedaly · 2014-01-09T18:03:03Z

common/djangoapps/terrain/stubs/lti.py

The superclass defines a send_response method that accepts a headers kwarg. You should use that to send the headers.

wedaly · 2014-01-09T18:16:33Z

Take another look at the StubHttpService base class -- you can re-use the helper methods defined there instead of re-implementing them in the subclass.
I don't understand why the stub needs different configuration for unit/acceptance/manual testing. For unit tests, you should use mock.patch to mock requests.post. I don't understand why manual testing is handled as a separate case.
Please check for pylint/pep8 style violations.
Rebase onto master and resolve merge conflicts.

Please let me know when you're ready for me to take another look.

wedaly · 2014-01-09T18:25:35Z

In terms of starting the server manually, you can add a short Python script that starts the server on a particular port. I'd recommend adding this directly in common/djangoapps/terrain/stubs/lti.py for now. It should look something like this:

 if __name__ == "__main__":
     service = StubLtiService()
     service.start()
     try:
          while true:
               pass
     except KeyboardInterrupt:
          service.shutdown()
          exit(0)

wedaly · 2014-01-10T13:59:37Z

It looks like the LTI module is setting the hostname for the callback URL here:

https://github.com/edx/edx-platform/blob/master/common/lib/xmodule/xmodule/lti_module.py#L284-L299

It's setting it based on the XBlock's self.system.hostname property. This, in turn, looks like it's configured using Django settings:

https://github.com/edx/edx-platform/blob/master/lms/djangoapps/courseware/module_render.py#L391

We have SITE_NAME set to localhost in the sandbox, so it should be safe to change it to localhost in dev and devstack settings as well. I'd prefer to configure this in settings rather than within the stub server.

olmar · 2014-02-03T16:03:46Z

@wedaly please continue review.

wedaly · 2014-02-03T20:38:24Z

common/djangoapps/terrain/stubs/lti.py

This looks like a private method; if so, it should have an underscore in front to denote that:

_create_content

made method to be private

wedaly · 2014-02-03T20:52:37Z

I made some style comments, and I'm still unsure about explicitly setting http/https for the test environment. You'll also need to rebase and fix merge conflicts.

Please let me know when you're ready for another round of review.

olmar · 2014-02-04T15:39:42Z

@auraz this PR involves some changes to LTI Module also. Please review.

olmar · 2014-02-04T16:07:44Z

@wedaly please continue review.

wedaly · 2014-02-04T21:18:08Z

👍 but please squash your commits before merging.

auraz · 2014-02-06T09:56:31Z

common/djangoapps/terrain/stubs/lti.py

a) add comment why we do this
b) please remove changes to quoting

auraz · 2014-02-06T10:02:46Z

lms/djangoapps/courseware/tests/test_lti_integration.py

same issue with quotes

auraz · 2014-02-06T10:03:37Z

Please fix small comments and 👍

auraz · 2014-02-06T10:08:23Z

lms/envs/acceptance.py

SITE_NAME = 'localhost:{}'.format(LETTUCE_SERVER_PORT)

Refactor stub implementation of LTI Provider. BLD-601.

* Add menu to ga_operation for ga_analyzer openedx#2039 (openedx#2088) * add role for old course viewer openedx#2062 (openedx#2087) * add role for old course viewer openedx#2062 * Change action for biz course by BetaTester role openedx#2062 * Construction of image server openedx#2025 (openedx#2106) * cherry-pick 8c8953f * Fix file upload in IE * Construction of image server openedx#2025 * add all keywords search in Student management openedx#2029 (openedx#2034) * Fix bug for before enrollment start in ga old course viewer openedx#2062 (openedx#2125) * fix. Construction of image server openedx#2025 (openedx#2117) * Modify message and css of enrollment for Course About openedx#2130 * Add a certificate list to user's profile page. openedx#2042 (openedx#2108) * Mod UT openedx#2130 * add PDF File Construction of image server openedx#2025 (openedx#2140) * add library option, and library links to the course. openedx#2001 (openedx#2124) * Invalid StudioPermissionsService object in API to show/save xblock settings in CMS. Randomized Content Block editor did not check Studio user's permissions * add library option, and library links to the course. openedx#2001 * fix. add all keywords search in Student management openedx#2029 (openedx#2034) (openedx#2157) * second fix. Construction of image server openedx#2025 (openedx#2158) * add library option, and library links to the course. openedx#2001 (openedx#2160) * third fix. Construction of image server openedx#2163 (openedx#2164) * Add filter by category for certificates on profile page openedx#2042 (openedx#2165) * Fix bug for add library option, and library links to the course. openedx#2162 openedx#2133 (openedx#2167) * Develop/dogwood/gacco201708 (openedx#2170) * Fixed bugs openedx#2039 (openedx#2112) * Fixed csv format openedx#2039 (openedx#2127) * Change to split download if there are many display items openedx#916 (openedx#2121) * Change to split download if there are many display items openedx#916 * Fix UT * Fix Review * Fix review2

singingwolfboy reviewed Dec 23, 2013
View reviewed changes

lms/djangoapps/courseware/features/lti_setup.py Outdated

Copy link

Contributor

singingwolfboy Dec 23, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't commit commented-out code to the repository.

wedaly reviewed Jan 9, 2014
View reviewed changes

wedaly reviewed Feb 3, 2014
View reviewed changes

auraz reviewed Feb 6, 2014
View reviewed changes

common/djangoapps/terrain/stubs/lti.py Outdated

Copy link

Contributor

auraz Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a) add comment why we do this
b) please remove changes to quoting

auraz reviewed Feb 6, 2014
View reviewed changes

lms/djangoapps/courseware/tests/test_lti_integration.py

Copy link

Contributor

auraz Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same issue with quotes

auraz reviewed Feb 6, 2014
View reviewed changes

lms/envs/acceptance.py Outdated

Copy link

Contributor

auraz Feb 6, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SITE_NAME = 'localhost:{}'.format(LETTUCE_SERVER_PORT)

Refactor stub implementation of LTI Provider. BLD-601.

79ffcb7

olmar added a commit that referenced this pull request Feb 11, 2014

Merge pull request #2029 from edx/oleg/lti_stub_refactor

1d517bd

Refactor stub implementation of LTI Provider. BLD-601.

olmar merged commit 1d517bd into master Feb 11, 2014

olmar deleted the oleg/lti_stub_refactor branch February 11, 2014 12:32

shimulch pushed a commit to open-craft/openedx-platform that referenced this pull request Jan 26, 2021

VB GP v2 (openedx#2029)

987d0c5

Refactor stub implementation of LTI Provider. BLD-601. #2029

Refactor stub implementation of LTI Provider. BLD-601. #2029

Uh oh!

Conversation

olmar commented Dec 23, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olmar commented Dec 30, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wedaly commented Jan 9, 2014

Uh oh!

wedaly commented Jan 9, 2014

Uh oh!

wedaly commented Jan 10, 2014

Uh oh!

olmar commented Feb 3, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wedaly commented Feb 3, 2014

Uh oh!

olmar commented Feb 4, 2014

Uh oh!

olmar commented Feb 4, 2014

Uh oh!

wedaly commented Feb 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

auraz commented Feb 6, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants