-
Notifications
You must be signed in to change notification settings - Fork 253
feat: Added data_share_consent field to order fullfillment notes #3939
feat: Added data_share_consent field to order fullfillment notes #3939
Conversation
a62074f
to
4b5bafa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good 👍
52d8ba2
to
6d7cabe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking Good 👍
@@ -1020,6 +1044,11 @@ def fulfill_product(self, order, lines, email_opt_in=False): | |||
) | |||
|
|||
try: | |||
self._create_enterprise_customer_user_consent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[clarification/curious] Say the create_enterprise_allocation
API endpoint throws an exception below, after already successfully creating a new DSC record for the user/course here. It seems here appears we'd be keeping the newly created DSC record around in our system even though we failed to actually make the GEAG allocation/enrollment.
Curious on the rationale for creating the DSC record prior to a successful create_enterprise_allocation
call to the GEAG API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assumption is that the consent api maybe more flaky than the GEAG api. Hence, if the consent fails to be recorded, we don't want the GEAG to be recorded as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious] If we create a DSC record but then fail to create the GEAG enterprise allocation, could there exist a mechanism to attempt to remove the newly created DSC record (essentially garbage collect DSC record if allocation failed)? Either way, likely not something to invest too much time into given this code path will eventually be replaced by the APIs in the EMET system.
[aside] I'd also be curious to hear why you think the consent API might be more flaky than the GEAG API? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adam: I think issue can also happen if we first create the GEAG enrollment and then create consent record. Lets say consent errors out after successful GEAG enrollment. What to do in that case? Should we rollback the GEAG enrollment? I think consent is mandatory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that if both the GEAG enrollment and consent record should be present then we should rollback transaction in case of error in any of the GEAG enrollment call or consent record call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets say consent errors out after successful GEAG enrollment. What to do in that case? Should we rollback the GEAG enrollment? I think consent is mandatory?
FWIW, I'm just clarifying the approach. I don't feel strongly that the DSC call needs to be after a successful GEAG allocation. Just asking for my own edification 😄
That said, when we call GEAG, we are already passing data_share_consent: true
in the GEAG API payload, which from my understanding should be sufficient from GEAG's perspective. I had thought our LPR reporting will be relying on the data_share_consent: true
passed to the GEAG enterprise allocation API, not so much an actual DSC record in our own database (this assumption may be incorrect).
Also, isn't consent conditionally required based on the enable_data_sharing_consent
config flag on the EnterpriseCustomer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adamstankiewicz ... always much appreciate your insights. I think we did not want to change the LPR pipeline with flags from GEAG. Hence, this consent api call ... you are correct that DSC should be garbage collected at some point. The frontend MFE will ensure that the consent checkbox is only visible for those learners who have the enable_data_sharing_consent config flag on their EnterpriseCustomer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnnagro @iloveagent57 I am not to familiar with the plan for GEAG<->DSC integration for LPR ... Can you review also?
We are currently creating the DSC records to enable LPR for these learners. However, api failure could result in a valid DSC without an associated allocation in GEAG. However, the DSC calls are idempotent ... so we expect to have the situation fixed whenever the allocation attempt is repeated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
6d7cabe
to
78d4007
Compare
78d4007
to
e837c0d
Compare
* build: Creating a missing workflow file `self-assign-issue.yml`. The .github/workflows/self-assign-issue.yml workflow is missing or needs an update to stay in sync with the current standard for this workflow as defined in the `.github` repo of the `openedx` GitHub org. * build: Creating a missing workflow file `add-remove-label-on-comment.yml`. The .github/workflows/add-remove-label-on-comment.yml workflow is missing or needs an update to stay in sync with the current standard for this workflow as defined in the `.github` repo of the `openedx` GitHub org. * build: Updating a missing workflow file `add-depr-ticket-to-depr-board.yml`. The .github/workflows/add-depr-ticket-to-depr-board.yml workflow is missing or needs an update to stay in sync with the current standard for this workflow as defined in the `.github` repo of the `openedx` GitHub org. * docs: Remove repo specific CONTRIBUTING.rst We now have a org wide CONTRIBUTING.md that points to our correct general contributing guidelines. We don't need repo specific ones that forward to other contributing docs. * fix: account for refunds in exec ed 2u redemption flow (#3920) * chore: add logging to include fulfillment details upon GEAG allocation exception * chore: quality * fix: Pick the right purchase from ios response (#3921) * fix: Pick the right purchase from ios response iOS response contain multiple purchases, instead of picking the first purchase, pick the one which have given product id and latest date. LEARNER-9261 * feat: Added Android refund api (#3922) * feat: Added Android refund api Like Apple android doesn't have callback for every refund. Therefore we have created an endpoint which we will hit daily through ecommerce worker. Learner-9149 * feat: Error if products in basket are already purchased (#3929) * feat: Error if products in basket are already purchased * refactor: Add tests, Improve error message * refactor: Update docstring * test: Increase coverage * chore: add logging to debug ent-6954 (#3931) * fix: Fix error in checkout api for mobile (#3934) * fix: Fix error in checkout api for mobile * fix: Return error in case of duplicate transaction_id for mobile (#3936) * fix: Return error in case of duplicate transactionID for mobile * refactor: Review feedback, add documentation * feat: Added course and expires field in product form on ecommerce dashboard (#3938) Forked catalogue app from oscar and added course and expire field in ProductForm. This change will enable to add Android sku from a same dashboard page. * fix: reorder JWT decoders (#3941) Reordered the JWT decoders to first use the standard library version, and then use the custom ecommerce decoder which uses multiple issuers. In this way, we can see if any JWTs cannot be decoded by that standard library version, and when and if we are ready to retire the custom JWT decoding code. See DEPR openedx/public-engineering#83 * fix: cached monitoring (#3942) Monitoring features such as use of the increment method, to increment a custom attribute, requires the CachedCustomMonitoringMiddleware. This has been added so the earlier calls to increment will function. * feat: add discount_jwt monitoring (#3944) Add monitoring for the discount JWT. * feat: Added data_share_consent field to order fullfillment notes (#3939) Co-authored-by: IrfanUddinAhmad <[email protected]> * chore: Switch from edx-sphinx-theme to sphinx-book-theme The edx-sphinx theme is being deprecated, and replaced with sphinx-book-theme. This removes references to the deprecated theme and replaces them with the new standard theme for the platform. * test: Add tests for Mobile IAP (#3937) * test: Add tests for mobile In-app purchases This reverts commit 54ea975. * fix: fix codecov error Codecov PyPI package was removed on 12 April and the recommended step is to migrate to codecov Github Action instead. * fix: add an exec ex 2u max application check to the checkout flow. ENT-7059 Also removes codecov from ci.yml workflow. * feat: add product entitlement info api (#3945) * fix: Updated format for data_share_consent field * docs: Update the contributing guidelines link. We're moving towards a single set of guidelines org-wide. * feat!: remove custom JWT decoding (#3943) * feat!: remove custom JWT decoding Removes the ecommerce custom JWT decoding, and replaces with the simple decoding from the edx-drf-extensions library. * fix: drop constraints and make upgrade * fix: handle major upgrade of django-crispy-forms The major upgrade of django-crispy-forms called for some changes related to bootstrap3 and dependencies. See https://github.com/django-crispy-forms/django-crispy-forms/blob/main/CHANGELOG.md#major-changes-and-migration-guide * fix: code coverage reporting Codecov no longer exists on PyPI, so switch to github action to run coverage report. --------- Co-authored-by: Muhammad Zubair <[email protected]> * fix: Course to have multiple seats with certificate_type attribute (#3950) * fix: Course to have multiple seats with certificate_type attribute * refactor: Modify SKU generation hash, add tests * test: Modify tests * temp: update JWT_DECODE_HANDER in devstack.py Jenkins job for building devstack images is temporarily broken. This should fix the devstack settings until this configuration change lands in an updated image: openedx-unsupported/configuration#6921 * feat: add native Dockerfile to create ansible free image * feat: add additional fields to EnterpriseLearnerOfferApiSerializer (#3963) * refactor: add logging to mobile IAP (#3962) * refactor: Improve exception handling for mobile IAP (#3969) * refactor: Improve exception handling for mobile IAP * refactor: pylint fixes * feat: Fix capture_context error on Payment MFE (#3965) * feat: Fix capture_context error on Payment MFE * feat: removed whitespace * feat: removed whitespace * feat: modified test case * feat: modified test case --------- Co-authored-by: Muhammad Zubair <[email protected]> * feat: Add enterprise_customer_name in the event metadata for offer usage braze emails. (#3972) * feat: Embargo check for subscription Programs (#3960) * fix: Enable TrackingMiddleware for Mobile IAP basket (#3977) * chore: updated Python requirements (edx-ecommerce-worker to version 3.3.3) (#3968) Co-authored-by: Muhammad Zubair <[email protected]> * fix: schedule upgrade-python-requirements monthly * fix: add edx-revenue-tasks to user_reviewers & remove team_reviewers Will create a Jira ticket instead of tagging all the members of @openedx/revenue-squad. * feat: add sf line item field to enterprise offers ENT-7013 * feat: Added ios refund callback (#3967) * feat: add SDN endpoints (#3985) * feat: add endpoint to run SDN check and return counts * feat: add SDNCheckFailure REST APi * fix: fix 500 on SDN for subscriptions (#3989) * fix: fix 500 on SDN for subscriptions * fix: pytest-selenium, pytest-variables, pyjwkest dependency issues (#3987) * feat: add coupon sf opp line item attribute * feat: Store price and currency for Mobile IAP (#3992) * feat: Store price and currency for mobile IAP * fix: return 200 on embargo failure to prevent downstream error (#3993) * feat: Make mobile IAP execute/ API atomic (#3995) * chore: added CODEOWNERS file (#3970) * refactor: Add logging to mobile IAP checkout/ API (#4000) * chore: django security patch 3.2.20 upgrade (#3999) * feat: Updates opportunity line item regex and tests (#3996) * feat: unenroll refunded android users daily (#4015) * feat: unenroll refunded android users daily Django management command to un-enroll refunded android users. This command will be run by Jenkins job daily. * feat: mail mobile team for a mobile course change in publisher (#4014) * feat: mail mobile team for a mobile course change in publisher This will fix any unknown change from publisher to a course having mobile seats. After this fix mobile team will see mail and adjust price of the course on playstore or appstore. In the longer run we want to replace this solution by changing the course price directly using mobile platform apis. LEARNER-9377 * fix: fixed coverage issue --------- Co-authored-by: Feanil Patel <[email protected]> Co-authored-by: Adam Stankiewicz <[email protected]> Co-authored-by: jawad khan <[email protected]> Co-authored-by: Moeez Zahid <[email protected]> Co-authored-by: Robert Raposa <[email protected]> Co-authored-by: irfanuddinahmad <[email protected]> Co-authored-by: IrfanUddinAhmad <[email protected]> Co-authored-by: Kshitij Sobti <[email protected]> Co-authored-by: Mohammad Ahtasham ul Hassan <[email protected]> Co-authored-by: Alex Dusenbery <[email protected]> Co-authored-by: Muhammad Zubair <[email protected]> Co-authored-by: Soban Javed <[email protected]> Co-authored-by: Jade Olivier <[email protected]> Co-authored-by: Saleem Latif <[email protected]> Co-authored-by: Shahroz Ahmad <[email protected]> Co-authored-by: Phillip Shiu <[email protected]> Co-authored-by: Phillip Shiu <[email protected]> Co-authored-by: Hamzah Ullah <[email protected]> Co-authored-by: jawad khan <[email protected]> Co-authored-by: Chris Pappas <[email protected]> Co-authored-by: Usama Sadiq <[email protected]>
Co-authored-by: IrfanUddinAhmad <[email protected]>
Description
This PR adds a data sharing consent flag to the order fulfillment notes. The flag will record the learner's acceptance of the data sharing consent while registering for third party hosted course content. The PR also uses the LMS consent apis to record the data sharing consent by the learner.
Supporting information
Jira: ENT-6794