MIT-LCP / physionet-build

The new PhysioNet platform.
https://physionet.org/
BSD 3-Clause "New" or "Revised" License
56 stars 20 forks source link

Restrcting the AccessPolicy options allowed during the project setup #2204

Closed Rutvikrj26 closed 6 months ago

Rutvikrj26 commented 6 months ago

Why this is required:

What this PR does:

bemoody commented 6 months ago

Use config, don't use os.environ.

This setting kind of goes together with ENABLE_FILE_DOWNLOADS_OPTION; I would list these two settings in the same place and perhaps name the new setting ENABLED_ACCESS_POLICIES (though "allowed" is also fine.)

I think it'd be better not to list the possible values in the physionet/settings/base.py.

Also the 'replace("_", " ").title()' (duplicating the logic of AccessPolicy.choices) doesn't really belong in the form class.

So it could be something more like:

    if settings.ALLOWED_ACCESS_POLICIES:
        self.fields['access_policy'].choices = [
            (value, label) for (value, label) in AccessPolicy.choices()
            if AccessPolicy(value).name in settings.ALLOWED_ACCESS_POLICIES
        ]

Or maybe it would be cleaner to define a method AccessPolicy.allowed_choices instead. (I don't think we want to change the behavior of AccessPolicy.choices because those choices are baked into migrations.)

Rutvikrj26 commented 6 months ago

Heyy @bemoody Thanks for reviewing!

I've updated the code based on your suggestions except one pending change which I would like to understand the reason better before implementing.

I think it'd be better not to list the possible values in the physionet/settings/base.py.

Wouldn't it be better to have that as a default value? The though was whichever deployment of Physionet wants to configure this setting, can just choose to add the environment variable, making it optional. Let me know your thoughts.

tompollard commented 6 months ago

We should also either: (1) Work out where we are going to document configurable settings or (2) Add the ALLOWED_ACCESS_POLICIES variable to the .env.example file.

tompollard commented 6 months ago

Thanks @Rutvikrj26. I added a couple of suggestions above. Please address them in a new PR if necessary!