fix: removal of temporary saml toggle#37651
Conversation
ktyagiapphelix2u
force-pushed
the
ktyagi/remove-toggle
branch
from
9311038 to
1e6958f
Compare
jcapphelix
left a comment
jcapphelix
left a comment
There was a problem hiding this comment.
Approved, add related PRs in description.
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
ktyagiapphelix2u
force-pushed
the
ktyagi/remove-toggle
branch
3 times, most recently
from
2f31a87 to
20ba3ee
Compare
ktyagiapphelix2u
force-pushed
the
ktyagi/remove-toggle
branch
from
20ba3ee to
599fb53
Compare
jcapphelix
left a comment
jcapphelix
left a comment
There was a problem hiding this comment.
Looks good to me, can go for further approvals.
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
| self.__create_saml_configurations__() | ||
|
|
||
| expected = "\nDone.\n1 provider(s) found in database.\n0 skipped and 1 attempted.\n1 updated and 0 failed.\n" | ||
| expected = "\nDone.\n2 provider(s) found in database.\n1 skipped and 1 attempted.\n1 updated and 0 failed.\n" |
There was a problem hiding this comment.
Can you explain why removal of the toggle results in an additional provider?
There was a problem hiding this comment.
Now, with the toggle gone, the base setup always creates a provider and configuration (the "setup" provider). When the test method also creates its own providers, the total number of providers in the database increases by one compared to before. That’s why, for example, the expected count changes from 1 to 2 providers in the test output.
There was a problem hiding this comment.
- I'm forgetting what "skipped" means and what "attempted" means in this message.
- I'm still not clear what the base setup creates a provider/configuration now with the toggle gone, but didn't before the toggle was gone. I'm still missing something.
There was a problem hiding this comment.
@ktyagiapphelix2u: I'm still waiting on a response here. I also don't understand what "1 updated" means. Nothing actually gets updated, right?
There was a problem hiding this comment.
@ktyagiapphelix2u: I continue to not get a response for this comment, and it affects much of the review, so I don't plan on proceeding until I hear back.
- I still want to know what "skipped" and "attempted" are, and whether or not these are leftover from when the command used to do something, rather than just report out? We probably shouldn't mess with anything in this PR, but maybe we should do a simple clean-up, if this is outdated?
- Should we drop the call to
self._create_saml_configurations()if there is already enough test data for this test? Same with some other tests? When do we actually need to call this? I still find that the test is more confusing than it needs to be, especially from a review perspective, wondering why the expectation needs to change.
There was a problem hiding this comment.
Apologies for missing that message earlier, which is why I wasn’t able to respond at the time. I have attached a documenet regrading your all the question and mystery behind this which i was able to find at the bottom please check. Thanks
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
| self.__create_saml_configurations__() | ||
|
|
||
| expected = "\nDone.\n1 provider(s) found in database.\n0 skipped and 1 attempted.\n1 updated and 0 failed.\n" | ||
| expected = "\nDone.\n2 provider(s) found in database.\n1 skipped and 1 attempted.\n1 updated and 0 failed.\n" |
There was a problem hiding this comment.
- I'm forgetting what "skipped" means and what "attempted" means in this message.
- I'm still not clear what the base setup creates a provider/configuration now with the toggle gone, but didn't before the toggle was gone. I'm still missing something.
…x-platform into ktyagi/remove-toggle
common/djangoapps/third_party_auth/management/commands/tests/test_saml.py
Outdated
Show resolved
Hide resolved
|
@robrap This is the document. |
Removes temporary rollout toggle ENABLE_SAML_CONFIG_SIGNAL_HANDLERS. The toggle was used to rollout a fix, and now the fix that uses the signal handlers is enabled by default. The only follow-up needed by anyone is to no longer set this toggle, which will no longer do anything.
3 participants
Description
Removal And Cleanup of temprory rollout toggle ENABLE_SAML_CONFIG_SIGNAL_HANDLERS from signal handlers
Private Ticket Link
https://2u-internal.atlassian.net/browse/BOMS-64