Add send-to-Google-Sheets support for B2B enrollment codes#3363
Open
Add send-to-Google-Sheets support for B2B enrollment codes#3363
Conversation
OpenAPI ChangesShow/hide ## Changes for v0.yaml:Unexpected changes? Ensure your branch is up-to-date with |
jkachel
force-pushed
the
jkachel/7931-add-b2b-gsheets-output
branch
from
4428dc9 to
d23141c
Compare
ruff doesn't like this but it'll generate errors in existing code.
…update all relevant contract sheets
…one would end up raising an exception
jkachel
force-pushed
the
jkachel/7931-add-b2b-gsheets-output
branch
from
00d45b6 to
910a0ad
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| total_updated += updated | ||
| total_errors += errors | ||
|
|
||
| queue_contract_sheet_update_post_save.delay(contract.id) |
There was a problem hiding this comment.
Bug: Saving a ContractPage incorrectly queues the same Google Sheet update task twice, causing redundant processing and wasted resources.
Severity: MEDIUM
Suggested Fix
Remove the redundant call to queue_contract_sheet_update_post_save.delay(contract.id) from the end of the ensure_enrollment_codes_exist function in b2b/api.py. The task is already correctly queued by the ContractPage.save() method, so this second call is unnecessary.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: b2b/api.py#L937
Potential issue: Saving a `ContractPage` object triggers two separate
`queue_contract_sheet_update_post_save` tasks for the same contract. One task is queued
directly by the `ContractPage.save()` method. A second, redundant task is queued
indirectly via a call chain: `save()` queues `queue_enrollment_code_check`, which calls
`ensure_enrollment_codes_exist`, which then queues the same update task again. This
duplication leads to wasted Celery worker resources, unnecessary Google Sheets API
calls, and could introduce race conditions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What are the relevant tickets?
Closes mitodl/hq#7931
Description (What does it do?)
Adds the ability to send a contract's enrollment codes to a Google Sheet.
This PR adds a couple of new fields to the
ContractPagemodel. These fields allow the user to specify the URL to a Google Sheet that we can throw codes into, and what worksheet (tab) within the document to use for this purpose. When set, the system will write enrollment codes to the sheet, and update the status of the individual codes as they're used.Each
ContractPagecan have its own individual Google Sheet to work in. Or, it can share a Sheet with other contracts and use a specified tab within the sheet. The integration doesn't really care as long as the sheet and tab are accessible, but some care should be taken to make sure you're not re-using a sheet/tab combination. The tab field defaults to "Sheet1", which is the name of the first tab that is created by default in an empty Google Sheet. Be sure you change this if you've changed the tab name.You can map a single sheet to a contract. To make things easier for folks managing the contract, the integration takes a URL to the sheet rather than a bare sheet ID (like the deferrals and refunds integrations do). The sheet must be writable by the account that was used to authorize MITx Online's access to the Google Drive APIs.
Codes are written to the configured sheet via a Celery task:
ensure_enrollment_codes_existis called.B2B_GSHEETS_UPDATE_FREQUENCYandB2B_GSHEETS_UPDATE_OFFSETsettings (default is to run every hour).The task will check to see if either the target Google sheet or tab field has changed in the ContractPage. If it has, then the task will assume this is a new sheet, and write out the codes along with a header, overwriting anything that may be in the sheet starting at
A1. If those fields haven't changed, then it updates the codes in the sheet nondestructively instead.Nondestructive updates allow the customer (contract owner) to modify the sheet without worry of data loss, within reason. For updates, the system checks the stored row position for each discount code to see if the code in the sheet matches the discount, and updates that row if it does. If it doesn't, then it scans for the code in the sheet starting from the header row, and updates the row if it finds the code. It writes the code data to a blank row if it can't find the code in the sheet at all. In the latter two cases, it will update the discount with the new stored row.
Updates will overwrite the 5 columns that aren't the discount code itself, so those columns are off-limits for editing. Any column beyond
Fis OK to edit. Deleting a code from the sheet, or modifying a code in the sheet, will cause it to be re-written.How can this be tested?
You'll need the Google Sheets integration set up. There are some docs on this in the ol-django repo: https://github.com/mitodl/ol-django/blob/main/src/google_sheets/README.md#developer-setup The main steps are under the "Developer Setup" section. You can ignore the third part (xPRO Drive folder) but the other 3 steps are essential if you've not set this up before.
Note
Getting this set up with the current state of the art for local development was sort of involved. In my case, it involved getting a paid ngrok account set up so I could reserve a hostname for the app and one subdomain of it for local Keycloak, and involved some further changes to my APISIX and Keycloak setup so that they would understand the ngrok hostnames.
There are some docs in the code that indicate that you can use a locally-generated token: https://github.com/mitodl/ol-django/blob/7207b8da0cba0f995fb1c5cf12fb9a814b0b2383/src/google_sheets/mitol/google_sheets/api.py#L108 I have not tried this but it might be easier to deal with than trying to get the full OAuth flow going.
Additionally the template doesn't seem to have a place for the Google verification tag anymore so I just added it manually to the base template.
You will need at least one contract that requires enrollment code (so, type "non-sso" or "code"). Ideally, test with a contract that has courses in it (and thus codes) and one that's brand new.
Testing should involve triggering all the events listed above. (For testing, it would be ideal to also set the
B2B_GSHEETS_UPDATE_FREQUENCYto a low number - maybe 60s or so.)Test the following scenarios:
You can trigger the tasks manually, or use the
ContractEnrollmentCodesSheetHandlerobject directly as well.Additional Context
The system doesn't update the sheet when attachments happen - instead, the default is to update all the sheets roughly every hour. It seemed like doing the updates immediately could cause some issues with using Google's APIs, especially as we add more and more contracts, so I opted to do a bulk update instead.
The integration uses the same base stuff that the deferrals and refunds processing code uses, but works a good bit differently. The refund/deferral code expects to only ever read a single sheet and this expects to work on any number of sheets. So, it's using the ol-django
google_sheetscode but really only to bootstrap pygsheets.The codes that are written are a sufficient number to fill the contract. In other words, if the contract is set for 100 seats, you'll get 100 codes written out, even if there's 9 courses in it (and thus 900 codes). This is similar to how the
b2b_codescommand works. However, the sheets integration will also purposefully find the codes that have been redeemed already and count them as part of the output set. (b2b_codesdefaults to just delivering usable codes.) The sheet will only display codes that have been redeemed for attachment to the contract.A field was also added to the
Discountmodel to keep track of where the code lands in the sheet, so it doesn't have to trawl through the entire sheet to update a code. (There can be potentially thousands of codes.) It will check to make sure the code is actually there; if it isn't, then it'll default to iterating through the sheet to find it.There's options in here to set the start row but we're not using the at the moment. The field in the Discount model that stores the row position is a character field so we can support cell indexes (i.e.
A1,C9, etc.) in the future.