Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changes for "can_request_access" feature #10877

Open
wants to merge 6 commits into
base: feature/institutional_access
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/institutions/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class InstitutionSerializer(JSONAPISerializer):
ser.CharField(read_only=True),
permission='view_institutional_metrics',
)
can_request_access = ser.CharField(read_only=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it clear this refers to a specific feature. Refactor to something like institutional_request_access_enabled

links = LinksField({
'self': 'get_api_url',
'html': 'get_absolute_html_url',
Expand Down
18 changes: 18 additions & 0 deletions osf/migrations/0026_institution_can_request_access.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 4.2.15 on 2024-12-27 14:08
Copy link
Contributor

Choose a reason for hiding this comment

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

Bunch this in with the other migrations on this branch, just to be tidy. Delete both this and 0025 and re-run makemigrations and use that file.


from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('osf', '0025_noderequest_requested_permissions_and_more'),
]

operations = [
migrations.AddField(
model_name='institution',
name='can_request_access',
field=models.BooleanField(db_index=True, default=False),
),
]
1 change: 1 addition & 0 deletions osf/models/institution.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class Institution(DirtyFieldsMixin, Loggable, ObjectIDMixin, BaseModel, Guardian
related_name='institutions'
)

can_request_access = models.BooleanField(default=False, db_index=True)
bodintsov marked this conversation as resolved.
Show resolved Hide resolved
is_deleted = models.BooleanField(default=False, db_index=True)
deleted = NonNaiveDateTimeField(null=True, blank=True)
deactivated = NonNaiveDateTimeField(null=True, blank=True)
Expand Down
2 changes: 1 addition & 1 deletion osf/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -646,7 +646,7 @@ def osf_groups(self):

def is_institutional_admin(self, institution):
group_name = institution.format_group('institutional_admins')
return self.groups.filter(name=group_name).exists()
return self.groups.filter(name=group_name).exists() and institution.can_request_access
Copy link
Contributor

Choose a reason for hiding this comment

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

It's difficult question where this AND check should be in the codebase. The institutional_admins permission may have other uses in the future that don't involve this feature, even though this is a quick fix and probably works for now you should move this check to the permissions classes that also check for this feature's request_type. UserMessagePermissions and InstitutionalAdminRequestTypePermission are better places to insert a conditional for this feature. is_institutional_admin can't "lie" about whether the user has that permission just because the feature is on/off.


def group_role(self, group):
"""
Expand Down
Loading