Honor verification required setting for professional courses in the receipt view #12470

jbzdak · 2016-05-16T09:48:44Z

Background

When creating a profesional course in CAT (/courses view ecommerce) administrator is given option to either enable or disable "Verification" feature for that course. This setting is honored on the LMS /dashboard view, however it is not honored on the /receipt view.

Only LMS is affected by this change.

Sandbox URL: http://pr12470.sandbox.opencraft.com/

Partner information: 3rd party-hosted open edX instance

Reviewers

Testing instructions

On sandbox

Login as a {{verified@example.com}} user and see following two receipts:

A receipt for course that does not require verification: http://pr12470.sandbox.opencraft.com/commerce/checkout/receipt/?orderNum=EDX-100007
A receipt for course that requires verification: http://pr12470.sandbox.opencraft.com/commerce/checkout/receipt/?orderNum=EDX-100008

On Devstack
Prepare the enviorment:

Install devstack using recent master
Setup a payment processor, paypal seems to be the easier one to set up. See setting-up PayPal for some hints
Create two new courses in Studio, name one of them "Professional with Verification Required" and another one "Professional no verification".
Start e-commerce and go to localhost:8002/courses, and add two courses, first one with Verification second one without it.

Reproduce the issue:

Register to the platform with a new user, and buy for both courses.
Observe that on Receipt for both courses you are asked to confirm your identity.

Verify the fix:

Check out this branch.
Go to the receipt pages for both courses, and observe that course with enabled verification still
asks user to verify:
However for course with verification disabled user can go straight to the dashboard

Setting up PayPal

To configure paypal add section from this gist to ecommerce/settings/private.py. And client_id and client_secret.

I also had problems releated to PayPal forcing TLS 1.2 for all urls (including Sandbox), if you'll get errors citing: 'sslv3 alert handshake failure', you'll need to update lib-ssl-*, openssl-* (and probably *curl*) in your devstack.

Settings

SANDBOX_ENABLE_ECOMMERCE: True

openedx-webhooks · 2016-05-16T09:48:50Z

Thanks for the pull request, @jbzdak! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=12470

mtyaka · 2016-05-31T06:58:27Z

👍

openedx-webhooks · 2016-05-31T07:21:58Z

Thanks for the pull request, @jbzdak! I've created OSPR-1273 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

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

supporting documentation
edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

verify the user.

shaunagm · 2016-09-29T14:46:53Z

@edx/ecommerce Does this need a second thumb?

(Also, sincere apologies @jbzdak for letting this sit without action for so long)

e-kolpakov · 2016-09-29T15:02:17Z

@shaunagm FYI, I've taken over this PR from @jbzdak and will be addressing your (and others') comments, so it would be helpful if you tagged me and not @jbzdak.

shaunagm · 2016-09-29T15:04:26Z

Will do, @e-kolpakov

e-kolpakov · 2016-09-30T08:28:10Z

@shaunagm Does this silence means "everything is right" or "we haven't looked at it yet"? :)

rlucioni · 2016-09-30T13:03:48Z

@e-kolpakov it means we haven't had a chance to look at it. Review of this PR is in our sprint, which ends next Monday. We'll take a look at it before then.

clintonb · 2016-10-03T17:40:05Z

@mjfrey has the receipt page been moved to edx/ecommerce? If not, when will that happen and what ticket is tracking the work?

clintonb · 2016-10-05T13:27:23Z

The decision has been made to not merge this. The receipt page is moving to edx/ecommerce, where this issue will be addressed.

shaunagm · 2016-10-20T13:51:44Z

Moving discussion from email to the relevant PR. It seems like there are two major options - making this fix as part of the refactoring/move to edx/ecommerce, and making this fix afterwards. What's your preference for how to proceed?

@clintonb @nedbat @antoviaque

(What's Deen's github handle? I want to tag them too.)

clintonb · 2016-10-20T14:37:55Z

Considering the uncertainty around when the receipt page will be moved, I'm inclined to merge this and move forward.

antoviaque · 2016-10-21T07:27:25Z

@clintonb That would be great - this way there is something in place to address it in the meantime. Thank you!

I'm reopening this PR then - let me know if there is anything else we need to do.

clintonb

This looks goods good aside from the string vs. boolean questions.

clintonb · 2016-10-21T15:33:04Z

lms/static/js/spec/commerce/receipt_view_spec.js

+                var requests, view, orderUrlFormat,
+                    actualRequestInThemedSite = requestInThemedSite;
+                if (typeof actualRequestInThemedSite === 'undefined') {
+                    actualRequestInThemedSite = 'False';


Why a string instead of an actual boolean?

@clintonb Most likely it would work with actualRequestInThemedSite (and corresponding parameters up the call chain) being a proper boolean, but this is closer to how LMS actually operates: it renders a python boolean into a HTML tag. When it happens, booleans are stred, and their python string representation is "True" and "False` (i.e. titlecase).

I would suggest keeping it as now for two reasons:

As I've said, it is closer to how LMS actually works

We maintain a fork with this exact commit applied - it would be nice to not introduce the code drift.

clintonb · 2016-10-21T15:35:05Z

lms/static/js/commerce/views/receipt_view.js

+        },
+
+        renderReceipt: function(data) {
+            var templateHtml = $('#receipt-tpl').html(),
                self = this,
                context = {
                    platformName: this.$el.data('platform-name'),
                    verified: this.$el.data('verified').toLowerCase() === 'true',
                    is_request_in_themed_site: this.$el.data('is-request-in-themed-site').toLowerCase() === 'true'


Same question regarding string versus boolean.

@clintonb Better ask @mattdrayer about this, but as far as I can tell, this.$el.data('is-request-in-themed-site') returns a string (just like other jQuery.data "getter" calls). Than, if what was stored in attribute was boolean there's a quirk:

When it is stored (either via data-something attribute in HTML or via jQuery.data call) it is serialized. Booleans serialize as "true" or "false"

When it is extracted by jQuery.data, it returns string, "true" or "false.

The thing is string "false" is still a trueish value if passed directly to JS bool function.

So, a check using === "true" is necessary.

antoviaque · 2016-10-24T07:58:54Z

@clintonb Thank you for the review! We'll schedule time from @e-kolpakov this week so he can look into your comments.

bradenmacdonald · 2016-10-26T05:37:20Z

Thanks for merging, @clintonb !

jbzdak changed the title ~~Honor verification required setting for professional courses~~ Honor verification required setting for professional courses in the receipt view May 16, 2016

jbzdak force-pushed the jbzdak/hide-verification branch from 9a94862 to d2d8e86 Compare May 16, 2016 11:21

jbzdak force-pushed the jbzdak/hide-verification branch from d2d8e86 to e42be45 Compare May 23, 2016 12:58

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage awaiting prioritization and removed needs triage labels May 31, 2016

e-kolpakov force-pushed the jbzdak/hide-verification branch 3 times, most recently from d70043c to 2958839 Compare August 30, 2016 14:54

When Proffesional course was created without verification don't

83e557d

verify the user.

e-kolpakov force-pushed the jbzdak/hide-verification branch from 2958839 to 83e557d Compare August 31, 2016 11:59

openedx-webhooks added engineering review and removed awaiting prioritization labels Oct 3, 2016

clintonb closed this Oct 5, 2016

antoviaque reopened this Oct 21, 2016

clintonb approved these changes Oct 21, 2016

View reviewed changes

clintonb merged commit 5f6ddf4 into openedx:master Oct 25, 2016

bradenmacdonald deleted the jbzdak/hide-verification branch October 26, 2016 05:37

clintonb mentioned this pull request Nov 7, 2016

[SOL-1954, SOL-1955, SOL-1956] New Receipt Page - Frontend openedx/ecommerce#978

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Honor verification required setting for professional courses in the receipt view #12470

Honor verification required setting for professional courses in the receipt view #12470

jbzdak commented May 16, 2016 •

edited by e-kolpakov

openedx-webhooks commented May 16, 2016

mtyaka commented May 31, 2016

openedx-webhooks commented May 31, 2016

shaunagm commented Sep 29, 2016

e-kolpakov commented Sep 29, 2016 •

edited

shaunagm commented Sep 29, 2016

e-kolpakov commented Sep 30, 2016

rlucioni commented Sep 30, 2016

clintonb commented Oct 3, 2016

clintonb commented Oct 5, 2016

shaunagm commented Oct 20, 2016

clintonb commented Oct 20, 2016

antoviaque commented Oct 21, 2016 •

edited

clintonb left a comment

clintonb Oct 21, 2016

e-kolpakov Oct 25, 2016

clintonb Oct 21, 2016

e-kolpakov Oct 25, 2016

antoviaque commented Oct 24, 2016

bradenmacdonald commented Oct 26, 2016

Honor verification required setting for professional courses in the receipt view #12470

Honor verification required setting for professional courses in the receipt view #12470

Conversation

jbzdak commented May 16, 2016 • edited by e-kolpakov

Testing instructions

Setting up PayPal

openedx-webhooks commented May 16, 2016

mtyaka commented May 31, 2016

openedx-webhooks commented May 31, 2016

shaunagm commented Sep 29, 2016

e-kolpakov commented Sep 29, 2016 • edited

shaunagm commented Sep 29, 2016

e-kolpakov commented Sep 30, 2016

rlucioni commented Sep 30, 2016

clintonb commented Oct 3, 2016

clintonb commented Oct 5, 2016

shaunagm commented Oct 20, 2016

clintonb commented Oct 20, 2016

antoviaque commented Oct 21, 2016 • edited

clintonb left a comment

Choose a reason for hiding this comment

clintonb Oct 21, 2016

Choose a reason for hiding this comment

e-kolpakov Oct 25, 2016

Choose a reason for hiding this comment

clintonb Oct 21, 2016

Choose a reason for hiding this comment

e-kolpakov Oct 25, 2016

Choose a reason for hiding this comment

antoviaque commented Oct 24, 2016

bradenmacdonald commented Oct 26, 2016

jbzdak commented May 16, 2016 •

edited by e-kolpakov

e-kolpakov commented Sep 29, 2016 •

edited

antoviaque commented Oct 21, 2016 •

edited