Skip to content

feat: make marketing email and research opt-in checkboxs selectively …#156

Merged
iloveagent57 merged 1 commit intorelease-ulmofrom
aed/optional-checkboxes
Mar 3, 2026
Merged

feat: make marketing email and research opt-in checkboxs selectively …#156
iloveagent57 merged 1 commit intorelease-ulmofrom
aed/optional-checkboxes

Conversation

@iloveagent57
Copy link
Member

@iloveagent57 iloveagent57 commented Mar 2, 2026

…ignorable

We want to support a flow for SSO-enabled Enterprise customers who have agreed off-platform that none of their learners will opt-in to marketing emails or sharing research data. This change proposes to do so by adding an optional field that, when enabled, disables the presence of the two checkboxes on this registration form and sets their values to false.

ENT-11401

Cherry-picked from: openedx#38042

…ignorable

We want to support a flow for SSO-enabled Enterprise customers who have
agreed off-platform that none of their learners will opt-in to marketing emails
or sharing research data. This change proposes to do so by
adding an optional field that, when enabled, disables the presence of
the two checkboxes on this registration form and sets their values to false.

ENT-11401
Copilot AI review requested due to automatic review settings March 2, 2026 18:01
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a SAML-provider-level configuration flag to suppress optional registration consent UI for specific enterprise SSO flows, ensuring learners don’t see marketing/research opt-in checkboxes during registration.

Changes:

  • Add skip_registration_optional_checkboxes to SAMLProviderConfig (with migration) and plumb it into third-party auth context.
  • Update registration form construction + logistration UI to hide the optional “research” toggle and suppress the marketing opt-in field for affected SAML providers.
  • Add tests validating marketing opt-in field visibility behavior under the new SAML flag, plus local SAML testing documentation.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
openedx/core/djangoapps/user_authn/views/utils.py Exposes skipRegistrationOptionalCheckboxes in third-party auth context.
openedx/core/djangoapps/user_authn/views/registration_form.py Implements SAML-driven suppression of marketing opt-in field and related overrides.
lms/templates/student_account/register.underscore Conditionally hides the optional-fields (“research”) toggle based on new context flag.
lms/static/js/student_account/views/RegisterView.js Passes new flag into the underscore template context.
common/djangoapps/third_party_auth/models.py Adds the new SAML provider configuration field.
common/djangoapps/third_party_auth/migrations/0014_samlproviderconfig_optional_email_checkboxes.py DB migration adding the boolean field.
openedx/core/djangoapps/user_authn/views/tests/test_saml_optional_checkboxes.py New tests for SAML-driven hiding behavior of marketing opt-in field.
openedx/core/djangoapps/user_authn/views/tests/test_logistration.py Updates expected third-party auth payload to include the new context flag.
common/djangoapps/third_party_auth/docs/how_tos/testing_saml_locally.rst Adds local SAML testing guide documentation.
.gitignore Adds local-dev ignore patterns (plus a duplicate .venv/ entry).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 340 to +355
def _is_field_visible(self, field_name):
"""Check whether a field is visible based on Django settings. """
return self._extra_fields_setting.get(field_name) in ["required", "optional", "optional-exposed"]
is_visible = self._extra_fields_setting.get(field_name) in ["required", "optional", "optional-exposed"]

# If SAML provider config wants to skip optional checkboxes, hide marketing_emails_opt_in
if is_visible and field_name == 'marketing_emails_opt_in':
saml_config = self._get_saml_provider_config()
if saml_config and saml_config.skip_registration_optional_checkboxes:
log.info(
"SAML provider %s has skip_registration_optional_checkboxes=True, "
"hiding marketing_emails_opt_in field",
saml_config.slug
)
return False

return is_visible
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When skip_registration_optional_checkboxes is enabled, this hides marketing_emails_opt_in by preventing the field from being added at all. That means the registration POST won’t include marketing_emails_opt_in, and downstream code that checks for the parameter’s presence (e.g., Segment email_subscribe / marketing_emails_opt_in properties in views/register.py) won’t record an explicit opt-out. Consider submitting an explicit false value (e.g., render as a hidden field with default false, or infer/set it server-side during registration for this provider) so “inferred as False” is consistently applied across consumers.

Copilot uses AI. Check for mistakes.
Comment on lines +467 to +477
# Try to find the SAML provider config
# First try with current_set(), then fall back to direct query
try:
return SAMLProviderConfig.objects.current_set().get(
slug=saml_provider_name
)
except SAMLProviderConfig.DoesNotExist:
# Fallback to direct query without current_set()
return SAMLProviderConfig.objects.get(
slug=saml_provider_name
)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_get_saml_provider_config falls back from objects.current_set().get(slug=...) to objects.get(slug=...). Because slug is not globally unique (and configs are site-scoped), this fallback can select the wrong provider (or raise MultipleObjectsReturned), and it bypasses the current-site/enabled/archived filtering that other third_party_auth code relies on. Prefer keeping the lookup within current_set() (or explicitly filtering by site and non-archived) and returning None if not found.

Suggested change
# Try to find the SAML provider config
# First try with current_set(), then fall back to direct query
try:
return SAMLProviderConfig.objects.current_set().get(
slug=saml_provider_name
)
except SAMLProviderConfig.DoesNotExist:
# Fallback to direct query without current_set()
return SAMLProviderConfig.objects.get(
slug=saml_provider_name
)
# Try to find the SAML provider config, restricted to the current set
return SAMLProviderConfig.objects.current_set().get(
slug=saml_provider_name
)

Copilot uses AI. Check for mistakes.
Comment on lines +348 to +352
log.info(
"SAML provider %s has skip_registration_optional_checkboxes=True, "
"hiding marketing_emails_opt_in field",
saml_config.slug
)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These log.info(...) lines will emit a log entry on every registration request for any IdP with skip_registration_optional_checkboxes=True. For large SSO-enabled enterprise customers, that could add significant log volume with limited diagnostic value. Consider reducing to debug level (or logging only when configuration can’t be found / is misconfigured).

Copilot uses AI. Check for mistakes.
Comment on lines 769 to +803
@@ -703,13 +783,31 @@ def _add_marketing_emails_opt_in_field(self, form_desc, required=False):
platform_name=configuration_helpers.get_value('PLATFORM_NAME', settings.PLATFORM_NAME),
)

# Default: checkbox is checked, field requirement follows the passed parameter
default_value = True
field_required = required
field_exposed = True

# Check if SAML provider wants to skip optional checkboxes
# This overrides both global settings and provider field overrides
saml_config = self._get_saml_provider_config()
if saml_config and saml_config.skip_registration_optional_checkboxes:
log.info(
"SAML provider %s has skip_registration_optional_checkboxes=True, "
"hiding field and setting default to False",
saml_config.slug
)
default_value = False # User opts out by default when field is skipped
field_required = False # Make field optional
field_exposed = False # Hide the field from the form

Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method hides marketing_emails_opt_in (returns False) when the SAML flag is set, but _add_marketing_emails_opt_in_field also contains logic to hide the field / change defaults for the same flag. Because the field won’t be added when _is_field_visible returns False, the logic and docstring in _add_marketing_emails_opt_in_field for the skip case are effectively unreachable. Recommend consolidating to a single mechanism (either omit the field entirely, or always add it but mark hidden/default false) and updating the docstring/tests accordingly.

Copilot uses AI. Check for mistakes.
</div>
<% if (!context.skipRegistrationOptionalCheckboxes) { %>
<div class="form-field checkbox-optional_fields_toggle">
<input type="checkbox" id="toggle_optional_fields" class="input-block checkbox"">
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input element has an extra double-quote at the end of the class attribute (class="input-block checkbox""), which makes the HTML invalid and can break attribute parsing in browsers. Remove the stray quote so the attribute value is well-formed.

Suggested change
<input type="checkbox" id="toggle_optional_fields" class="input-block checkbox"">
<input type="checkbox" id="toggle_optional_fields" class="input-block checkbox">

Copilot uses AI. Check for mistakes.
Comment on lines +14 to +16

log = logging.getLogger(__name__)

Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log = logging.getLogger(__name__) is declared but never used in this test module. This can fail linting (and is extra noise). Remove it or use it (e.g., for assertions via assertLogs) if logging is intended to be part of the test.

Copilot uses AI. Check for mistakes.
Comment on lines +233 to +237
def test_marketing_checkbox_still_optional_when_config_false(self, mock_is_enabled):
"""
Test that when SAML provider config has skip_registration_optional_checkboxes=False,
the global REGISTRATION_EXTRA_FIELDS setting is used (required in this case).
"""
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test name test_marketing_checkbox_still_optional_when_config_false conflicts with what it asserts: the docstring says the field should follow the global setting (required), and the test asserts required is True. Rename the test to reflect the behavior being verified (e.g., that the global requirement is honored when the SAML flag is False).

Copilot uses AI. Check for mistakes.
@iloveagent57 iloveagent57 enabled auto-merge (rebase) March 2, 2026 19:21
@iloveagent57 iloveagent57 merged commit 1851ebd into release-ulmo Mar 3, 2026
68 checks passed
@iloveagent57 iloveagent57 deleted the aed/optional-checkboxes branch March 3, 2026 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants