This repository has been archived by the owner on Nov 4, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
feat: Added data_share_consent field to order fullfillment notes #3939
feat: Added data_share_consent field to order fullfillment notes #3939
Changes from all commits
e837c0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 thedata_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 theEnterpriseCustomer
?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.