Add handful of events to the Segment.io whitelist - #263
Conversation
|
@wedaly any idea why the build failed? Looks like tests passed and pylint and pep8 are happy. |
There was a problem hiding this comment.
Isn't this going to change what's going into the tracking logs too? We don't want to mess with that without a really good reason.
There was a problem hiding this comment.
Yes. This change mirrors the way information is sent to the tracking logs by sequentials. I can instead put direct calls to the Segment.io API here, or only modify the information sent to Segment.io by the Logger.
|
I would try rebasing master to generate a new build, and see if that fixes the failures (that's an easy thing to try when your build fails for seemingly no reason) |
|
The build is still failing after performing a rebase to master. It looks like tests are still passing, and pylint and pep8 are good too. @wedaly, any suggestions? |
…er case where data passed is not a dictionary
|
I think the build should pass now. I wrote a Jasmine test earlier to check that Segment.io was being called as expected, but didn't change it after modifying the Logger. I've changed it and added an additional case so that I cover both cases we could see for data being passed to Segment.io's API. |
|
Turns out that wasn't enough. Can anyone recommend a solution? |
|
Mystery solved. The build is failing because of a Jasmine test which verifies that the |
|
@dianakhuang, could you please take a look at this? Victor is busy. |
|
This looks good to me. You mentioned that this was addressing one issue and might address https://edx-wiki.atlassian.net/browse/LMS-487. Can you provide the ticket for the first issue and, if my understanding is correct, explain in the ticket for 487 how this PR might affect it? |
|
The primary objective of this PR is not to address an issue per se, but to perform a conservative expansion of the whitelist of events being routed to Segment.io, as described here: https://edx-wiki.atlassian.net/browse/LMS-512. It was in the process of doing this that I discovered the duplicate Logger call. I'll go ahead and note on the ticket for 487 how this PR might affect it. I'm not sure that this resolves the issue described in 487, since no one is quite sure what Jim meant by "repeated data in the tracking logs." However, calling the Logger twice in a row would certainly result in duplicate data. |
|
OK makes sense. Just wanted to clarify our in person conversation in a more permanent location. Thanks 🐳 🐙 🌵 |
|
Looks good. 😎 |
Add handful of events to the Segment.io whitelist
Acceptance rebase 20140926
from Stanford-Online:giulio/actually-remove-pdf-download-open into Stanford-Online:master
fixbug enrollment
This PR adds MFE API. This is part of the work that is being done to obtain the MFE Runtime Configurations and that has been discussed in the BTR WG. Discussion: https://discuss.openedx.org/t/how-to-use-microfrontend-in-a-multitenant-instance/6936/14?u=mafermazu MFE Runtime configuration - eduNEXT: https://docs.google.com/document/d/1-FHIQmyeQZu3311x8eYUNMru4JX7Yb3UlqjmJxvM8do/edit?usp=sharing feat: add lms setting to set mfe config cache (#262) Co-authored-by: María Fernanda Magallanes Z <maria.magallanes@edunext.co> feat: make mfe config api disabled by default (#263) * feat: make mfe config api disabled by default * fix: simple is better than complex test: add mfe config tests (#264) * test: add mfe config tests * test: fix it and simplify it * test: correct pylint issues fix: correct pep 8 violations fix: add mfe api unit test in github workflow fix: correct unit tests refactor: move mfe api to lms fix: try mfe api urls without regex fix: add app_namespace in lms urls fix: try url without conditional Revert "fix: try url without conditional" This reverts commit 694aab546134b4bd9ad2642e24927b42cac24459. fix: set enable_mfe_config_api feature to true in the tests test: try to add failed test case Revert "test: try to add failed test case" This reverts commit cee6bf656ab1b96492b0b6199ddff32a6d6a65bd. docs: improve explanation and documentation fix: ensure the response is a json object refactor: be consistent with the variable names fix: allow overriding mfe api config cache timeout in production fix: handle 404 response in view refactor: use a guard instead if-else feat: add the possibility to show mfe specific config
Tell them about verified certificates and link to ecommerce. Also fixes AA-376 by handling the no-verified-mode-to-upgrade-to case.
…ieldsRequested filter (#263) Removes the direct import of get_enterprise_readonly_account_fields from openedx.features.enterprise_support.utils in accounts/api.py and replaces it with a call to the AccountSettingsReadOnlyFieldsRequested openedx-filter. Adds the filter to OPEN_EDX_FILTERS_CONFIG with the enterprise.filters.accounts.AccountSettingsEnterpriseReadOnlyFieldsStep pipeline step. Updates tests to mock the filter instead of the old enterprise_support imports. ENT-11510 Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
4 participants
This is a conservative expansion of the whitelist of events being routed through Segment.io. I modified the logger calls because the Segment.io API requires that dictionaries be passed to it. @sarina and @shnayder, please review?