Skip to content

fix: updates get provider from pipeline wrong parameter - #27079

Closed
nizarmah wants to merge 2 commits into
openedx:masterfrom
open-craft:nizar/remove_duplicate_code
Closed

fix: updates get provider from pipeline wrong parameter#27079
nizarmah wants to merge 2 commits into
openedx:masterfrom
open-craft:nizar/remove_duplicate_code

Conversation

@nizarmah

@nizarmah nizarmah commented Mar 19, 2021

Copy link
Copy Markdown
Contributor

Description

Previously, the provider was being retrieved from the pipeline using common.djangoapps.third_party_auth.provider.Registry.get_from_pipeline.

After some investigation into how the method works, it turned out that it used ProviderConfig.is_active_for_pipeline to check if the backend is active in the pipeline.

That method, compares ProviderConfig.backend_name with pipeline['backend'] which indicates that the pipeline's backend name should be passed and not the pipeline's backend class.

This pull request also removes duplicate code for saml provider check. A SAML provider check has already been added to third_party_auth utils in b99a64c. This change simply removes the old is_provider_saml method and replaces it with the new is_saml_provider.

Supporting information

Testing instructions

Not sure... The pull request that introduced that change has no description, so I have no idea how to test it.

A SAML provider check has already been added to third_party_auth utils in b99a64c

This change simply removes the old `is_provider_saml` method and replaces it with the new `is_saml_provider`
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @nizarmah! I've created OSPR-5682 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket:

  • supporting documentation
  • Open edX discussion forum threads
  • timeline information ("this must be merged by XX date", and why that is)
  • partner information ("this is a course on edx.org")
  • any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Mar 19, 2021
Previously, the provider was being retrieved from the pipeline using `common.djangoapps.third_party_auth.provider.Registry.get_from_pipeline`.

After some investigation into how the method works, it turned out that it used `ProviderConfig.is_active_for_pipeline` to check if the backend is active in the pipeline.

That method, compares `ProviderConfig.backend_name` with `pipeline['backend']` which indicates that the pipeline's backend name should be passed and not the pipeline's backend class.
@nizarmah
nizarmah force-pushed the nizar/remove_duplicate_code branch from e4c497e to 3c0e083 Compare March 19, 2021 15:09
@edx-status-bot

Copy link
Copy Markdown

Your PR has finished running tests. The following contexts failed:

  • jenkins/python

@nizarmah

Copy link
Copy Markdown
Contributor Author

Closing this pull request because I'm not sure what the test should be actually resulting in, and I don't want to adjust the test without knowing why I did an adjustment.

@openedx-webhooks

Copy link
Copy Markdown

@nizarmah Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U rejected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants