Skip to content

feat: add AccountSettingsEnterpriseReadOnlyFieldsStep pipeline step#2544

Merged
kiram15 merged 2 commits intomasterfrom
pwnage101/ENT-11510
Apr 6, 2026
Merged

feat: add AccountSettingsEnterpriseReadOnlyFieldsStep pipeline step#2544
kiram15 merged 2 commits intomasterfrom
pwnage101/ENT-11510

Conversation

@pwnage101
Copy link
Copy Markdown
Contributor

@pwnage101 pwnage101 commented Mar 4, 2026

Implements the AccountSettingsEnterpriseReadOnlyFieldsStep PipelineStep for the org.openedx.learning.account.settings.read_only_fields.requested.v1 openedx-filter. When a user is linked to an enterprise customer whose SSO IdP has sync_learner_profile_data enabled, the step adds ENTERPRISE_READONLY_ACCOUNT_FIELDS to the readonly_fields set. The name field is only included when a UserSocialAuth record exists for the IdP backend.

ENT-11510

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com


Blocked by:

Blocks:

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@kiram15 kiram15 force-pushed the pwnage101/ENT-11510 branch 2 times, most recently from 50361c6 to 304a409 Compare March 26, 2026 17:07
fn=lambda request, *args, **kwargs: kwargs.get('enterprise_customer_uuid'),
)
def delete_admin(self, request, enterprise_customer_uuid=None, id=None): # pylint: disable=redefined-builtin,unused-argument
def delete_admin(self, request, enterprise_customer_uuid=None, id=None): # pylint: disable=redefined-builtin
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lint error for unnecessary pylint ignore

@kiram15 kiram15 marked this pull request as ready for review March 26, 2026 22:26
@kiram15 kiram15 force-pushed the pwnage101/ENT-11510 branch 2 times, most recently from ffa236d to 186d67a Compare March 30, 2026 16:53
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 left a comment

Choose a reason for hiding this comment

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

Apologies, but I took a deeper look at the code and found many things I don't like!

Copy link
Copy Markdown

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 an edx-enterprise pipeline step that participates in the org.openedx.learning.account.settings.read_only_fields.requested.v1 openedx-filter to mark certain account settings fields as read-only for learners whose enterprise SSO IdP is configured to sync profile data.

Changes:

  • Introduce AccountSettingsReadOnlyFieldsStep to add ENTERPRISE_READONLY_ACCOUNT_FIELDS (conditionally including name based on social auth presence).
  • Add unit tests covering common branches (no enterprise link, no IdP, no TPA, sync disabled, name excluded without social auth).
  • Add openedx-filters dependency and bump package version/changelog.

Reviewed changes

Copilot reviewed 10 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
enterprise/filters/accounts.py Implements the new PipelineStep that computes enterprise-driven read-only account fields.
tests/filters/test_accounts.py Adds unit tests for the pipeline step’s main decision branches.
requirements/base.in Adds openedx-filters as a runtime dependency.
requirements/test-master.txt Pins/records openedx-filters in compiled test requirements.
requirements/test.txt Updates compiled dependency provenance comments related to openedx-filters.
requirements/dev.txt Updates compiled dev requirements to include openedx-filters.
requirements/doc.txt Updates compiled doc requirements to include openedx-filters.
requirements/edx-platform-constraints.txt Updates a pinned dependency version during constraints sync.
enterprise/__init__.py Bumps package version to 6.8.4.
CHANGELOG.rst Adds the 6.8.4 release entry for this feature.

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

@kiram15 kiram15 force-pushed the pwnage101/ENT-11510 branch 3 times, most recently from 5da2567 to 013bf62 Compare March 31, 2026 04:26

fake_get_ec.return_value = enterprise_customer
assert handle_enterprise_logistration(backend, self.user) is None
assert handle_enterprise_logistration.__wrapped__(backend, self.user) is None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Had to use .wrapped to bypass the @partial decorator and test the business logic directly

@kiram15 kiram15 force-pushed the pwnage101/ENT-11510 branch from 013bf62 to d2f1767 Compare March 31, 2026 04:59
@kiram15 kiram15 force-pushed the pwnage101/ENT-11510 branch from d2f1767 to fea50a5 Compare March 31, 2026 15:46
# by the following command:
#
# make upgrade
# pip-compile --output-file=requirements/doc.txt requirements/doc.in
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nit: this will just be immediately un-done---maybe just keep this make upgrade.

@pwnage101
Copy link
Copy Markdown
Contributor Author

Just the Nit above and the fundamental question about matching against all vs. just one provider: #2544 (comment)

The rest LGTM!

Copy link
Copy Markdown

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

Copilot reviewed 14 out of 16 changed files in this pull request and generated 3 comments.


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

@pwnage101 pwnage101 changed the title feat: add AccountSettingsReadOnlyFieldsStep pipeline step feat: add AccountSettingsEnterpriseReadOnlyFieldsStep pipeline step Apr 1, 2026
@kiram15 kiram15 force-pushed the pwnage101/ENT-11510 branch from cb400fb to 109a270 Compare April 1, 2026 16:20
@kiram15 kiram15 force-pushed the pwnage101/ENT-11510 branch from 109a270 to 797bdc0 Compare April 1, 2026 16:27
Copy link
Copy Markdown
Contributor Author

@pwnage101 pwnage101 left a comment

Choose a reason for hiding this comment

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

✅ LGTM!

@kiram15 kiram15 merged commit bd0c369 into master Apr 6, 2026
11 checks passed
@kiram15 kiram15 deleted the pwnage101/ENT-11510 branch April 6, 2026 17:43
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.

4 participants