Skip to content

Valera/lti graded additional tests 2 - #1811

Merged
olmar merged 1 commit into
masterfrom
valera/lti_graded_additional_tests_2
Dec 17, 2013
Merged

Valera/lti graded additional tests 2#1811
olmar merged 1 commit into
masterfrom
valera/lti_graded_additional_tests_2

Conversation

@valera-rozuvan

@valera-rozuvan valera-rozuvan commented Dec 2, 2013

Copy link
Copy Markdown
Contributor

Includes new and updated Python tests for the LTI module. LTI must use HTTPS for lis_outcome_service_url.

BLD-564

Reviewers

@valera-rozuvan valera-rozuvan mentioned this pull request Dec 2, 2013
Closed
@ghost ghost assigned singingwolfboy Dec 2, 2013
singingwolfboy
singingwolfboy reviewed Dec 2, 2013
View reviewed changes
Comment thread common/lib/xmodule/xmodule/tests/test_lti_unit.py Outdated

@singingwolfboy singingwolfboy Dec 2, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Did you mean to comment this line out? Perhaps it should just be removed.

@olmar olmar Dec 3, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removed.

@valera-rozuvan

valera-rozuvan commented Dec 5, 2013

Copy link
Copy Markdown
Contributor Author

@auraz , @jmclaus , @singingwolfboy please finish with reviewing this PR. If all OK, give your thumbs up. I will squash all commits into 1, rebase, and merge.

@jmclaus

jmclaus commented Dec 5, 2013

Copy link
Copy Markdown

@valera-rozuvan Sure but could you please look at my comments first? Thanks.

@valera-rozuvan

valera-rozuvan commented Dec 5, 2013

Copy link
Copy Markdown
Contributor Author

@jmclaus Sorry. = ) Forgot the one "this.el.find('.lti') should be cached.". Doing now.

@jmclaus

jmclaus commented Dec 5, 2013

Copy link
Copy Markdown

@valera-rozuvan Good. And your call on using requireJS or not.

auraz
auraz reviewed Dec 5, 2013
View reviewed changes
Comment thread common/lib/xmodule/xmodule/lti_module.py Outdated

@auraz auraz Dec 5, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

data -> _

@valera-rozuvan valera-rozuvan Dec 5, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/edx/edx-platform/wiki/Python-Guidelines

For unused args, you can prefix the arguments with _ to mark them as unused (as convention), and pylint will accept that.

@valera-rozuvan valera-rozuvan Dec 5, 2013

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@auraz Maybe data -> _data ?

@valera-rozuvan

valera-rozuvan commented Dec 5, 2013

Copy link
Copy Markdown
Contributor Author

@jmclaus I have decided to use RequireJS. You are right that we should try to standardize the process of writing JavaScript code for XModules.

auraz
auraz reviewed Dec 5, 2013
View reviewed changes
Comment thread common/lib/xmodule/xmodule/js/src/lti/01_lti.js Outdated

@auraz auraz Dec 5, 2013

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

pass error from server here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@singingwolfboy singingwolfboy

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@valera-rozuvan @jmclaus @olmar @singingwolfboy @auraz