Skip to content

feat: add AccountSettingsReadOnlyFieldsRequested filter#334

Open
pwnage101 wants to merge 2 commits intoopenedx:mainfrom
pwnage101:pwnage101/ENT-11510
Open

feat: add AccountSettingsReadOnlyFieldsRequested filter#334
pwnage101 wants to merge 2 commits intoopenedx:mainfrom
pwnage101:pwnage101/ENT-11510

Conversation

@pwnage101
Copy link

@pwnage101 pwnage101 commented Mar 4, 2026

@pwnage101 pwnage101 force-pushed the pwnage101/ENT-11510 branch from 1443652 to a2bae8d Compare March 4, 2026 22:59
@pwnage101 pwnage101 marked this pull request as ready for review March 16, 2026 21:14
@felipemontoya
Copy link
Member

@pwnage101 I will try to review this afternoon. I know the big goal is to get rid of this import in the platform from openedx.features.enterprise_support.utils import get_enterprise_readonly_account_fields. Is there anything else about ENT-11510 that I should know?

Copy link
Member

@felipemontoya felipemontoya left a comment

Choose a reason for hiding this comment

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

There are some non conventional behaviours that should be easy to correct.

set: the (possibly expanded) set of read-only field names.
"""
data = super().run_pipeline(readonly_fields=readonly_fields, user=user)
return data.get("readonly_fields", set())
Copy link
Member

Choose a reason for hiding this comment

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

It is customary for filters to return the complete list of inputs that where passed to the filter.

I understand the intent, it would be a strange use case to have a filter such as AccountSettingsReadOnlyFieldsRequested return a different user object, but this is breaking the stablished pattern.

The filter should return something like:

        return (
            data.get("readonly_fields"),
            data.get("user"),
        )

On the other hand, you don't need to return an empty set() as the fallback. The only way for this to happen is for your implemented step to remove the key "readonly_fields" but that would only happen if the step is broken or if by design it is trying to break the pipeline and trigger some try-catch block. Returning a default here would interfere with the accepted mechanism.

Tests for the AccountSettingsReadOnlyFieldsRequested filter.
"""

def test_run_filter_returns_empty_set_unchanged_when_no_pipeline(self):
Copy link
Member

Choose a reason for hiding this comment

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

Most tests for filters are really easy because they only test that whatever you pass in them is returned back when the step pipeline is empty. That should be enough when you take the filter definition changes into account.

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