Changes for "can_request_access" feature #10877

bodintsov · 2024-12-30T10:34:33Z

Purpose

Add to admin "can_request_access" option to the institution
API Changes for "can_request_access" feature

Done 2 tasks (ENG-6779) and (ENG-6780) within one PR, due to tickets being related to each other

Changes

TBD

QA Notes

TBD

Documentation

TBD

Side Effects

TBD

Ticket

(https://openscience.atlassian.net/browse/ENG-6779)
(https://openscience.atlassian.net/browse/ENG-6780)

brianjgeiger

The changes so far look good with one small change, but we're missing the admin app functionality.

osf/models/institution.py

brianjgeiger · 2024-12-30T14:48:35Z

Oh, and also tests need to be fixed.

brianjgeiger · 2024-12-30T14:59:31Z

While you're in there fixing tests, you might as well add some new ones to cover the new field.

bodintsov · 2024-12-30T16:03:03Z

The changes so far look good with one small change, but we're missing the admin app functionality.

@brianjgeiger On the OSFadmin app, on the page of an institution, there is a possibility to change the can_request_access field. Maybe I missed something, and it needs to be done elsewhere?

brianjgeiger · 2024-12-30T16:21:15Z

On the OSFadmin app, on the page of an institution, there is a possibility to change the can_request_access field.

Oh, very good. Could take a screenshot and put it in the PR description, please?

bodintsov · 2024-12-30T21:58:51Z

Oh, very good. Could take a screenshot and put it in the PR description, please?

Yes sure, I added a screenshot

brianjgeiger · 2025-01-02T02:17:35Z

@bodintsov Looks like tests are still failing.

brianjgeiger

Just a couple of questions about an unexpected set of changes.

framework/email/tasks.py

Johnetordoff · 2025-01-03T14:22:34Z

@brianjgeiger @bodintsov is having some failing tests due to my changing of the sendgrid method, heres the fails on the feature branch https://github.com/CenterForOpenScience/osf.io/actions/runs/12380790854/job/34557929049?pr=10884. I told him to adjust the tests because that is working on staging.

bodintsov · 2025-01-04T09:08:34Z

I cant fix tests because the add_category method does not work. It is not assigning categories to the Mail object. Without fixing _send_with_sendgrid it is impossible to set proper tests.
Have also needed to add crutch to assert Email object to str.

brianjgeiger · 2025-01-06T13:42:14Z

@bodintsov Okay, understood. This overall looks good, but I see some likely unrelated test failures that will need to be handled one way or another before this can be merged. I'm checking with another team on Slack to see if this is related to their work, and we'll figure out where to go from here when they respond.

brianjgeiger · 2025-01-07T14:12:58Z

@bodintsov Okay, since you're fixing the broken tests in another PR, and we've already merged things that have those broken tests in, I will approve this once the merge conflicts are fixed, then let @Johnetordoff do a pass.

Johnetordoff

You got the goal of the assignment correct, but there's some changes that are necessary before merging. If you have some more questions ping me on Slack, if something seems odd or too difficult, it's probably just a misunderstanding. Good first draft. Thanks!

Johnetordoff · 2025-01-07T20:30:35Z

api/institutions/serializers.py

@@ -41,6 +41,7 @@ class InstitutionSerializer(JSONAPISerializer):
        ser.CharField(read_only=True),
        permission='view_institutional_metrics',
    )
+    can_request_access = ser.CharField(read_only=True)


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

Johnetordoff · 2025-01-07T20:32:40Z

osf/migrations/0026_institution_can_request_access.py

@@ -0,0 +1,18 @@
+# Generated by Django 4.2.15 on 2024-12-27 14:08


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.

Johnetordoff · 2025-01-07T20:34:49Z

osf_tests/factories.py

@@ -257,10 +257,14 @@ class InstitutionFactory(DjangoModelFactory):
    email_domains = FakeList('domain_name', n=1)
    orcid_record_verified_source = ''
    delegation_protocol = ''
+    can_request_access = False


I would default this to true, just because it's going to true going forward, but I'm not sure if it will break a bunch of tests consider using True here.

I would leave this as False, please. We don't want Product to have to turn this off for every institution to start with. We can always change it to True after launch once it goes fully into use.

Johnetordoff · 2025-01-07T20:35:50Z

osf_tests/factories.py


    class Meta:
        model = models.Institution

+    @classmethod
+    def _create(cls, *args, **kwargs):


I don't know what the point of doing this is, write a comment/docstring explaining or remove it.

Johnetordoff · 2025-01-07T20:36:51Z

tests/framework_tests/test_email.py

-        assert len(mock_mail.return_value.category) == 2
-        assert mock_mail.return_value.category[0].get() == category1
-        assert mock_mail.return_value.category[1].get() == category2
+        # Added this crutch as test fail due to improper work of method `add_category`. This method do not add categories


I think we cleared up all these email tests, seems like you should be able to remove this lines.

Johnetordoff · 2025-01-07T20:46:29Z

osf/models/user.py

@@ -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


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.

Johnetordoff · 2025-01-07T20:47:48Z

api_tests/users/views/test_user_message_institutional_access.py

@@ -89,6 +110,26 @@ def test_institutional_admin_can_create_message(self, mock_send_mail, app, insti
        assert 'Requesting user access for collaboration' in mock_send_mail.call_args[1]['message_text']
        assert user_message._id == data['id']

+    @mock.patch('osf.models.user_message.send_mail')
+    def test_institutional_admin_can_not_create_message(self, mock_send_mail, app, institutional_admin_on_institution_without_access,


There should be a test for the request node endpoint too.

…terForOpenScience/osf.io into feature/institutional_access * 'feature/institutional_access' of https://github.com/CenterForOpenScience/osf.io: Changes for "can_request_access" feature (CenterForOpenScience#10877)

…terForOpenScience/osf.io into institutional-access-insti-admin-validation * 'feature/institutional_access' of https://github.com/CenterForOpenScience/osf.io: Changes for "can_request_access" feature (CenterForOpenScience#10877) # Conflicts: # api/requests/permissions.py # api/users/permissions.py # osf/migrations/0025_contributor_is_curator_and_more.py

bodintsov marked this pull request as ready for review December 30, 2024 10:34

brianjgeiger requested changes Dec 30, 2024

View reviewed changes

osf/models/institution.py Outdated Show resolved Hide resolved

brianjgeiger reviewed Jan 2, 2025

View reviewed changes

framework/email/tasks.py Outdated Show resolved Hide resolved

framework/email/tasks.py Outdated Show resolved Hide resolved

brianjgeiger requested a review from Johnetordoff January 7, 2025 14:11

Johnetordoff requested changes Jan 7, 2025

View reviewed changes

bodintsov added 6 commits January 8, 2025 20:58

Changes for "can_request_access" feature

c4cebd8

fixed tests, added new tests, removed index on can_request_access

b9c92b8

fixed tests

edbde18

remove unsued import

3319392

reverted changes in tasks

aad9717

fixed comments

c8e345c

bodintsov force-pushed the feature/add_field_to_institutional_model branch from 7a7de42 to c8e345c Compare January 8, 2025 18:59

bodintsov added 2 commits January 8, 2025 23:39

fixed test

bdf304e

fixed failed assertion

7bbb40e

brianjgeiger approved these changes Jan 9, 2025

View reviewed changes

Johnetordoff approved these changes Jan 9, 2025

View reviewed changes

brianjgeiger merged commit d9540f3 into CenterForOpenScience:feature/institutional_access Jan 9, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for "can_request_access" feature #10877

Changes for "can_request_access" feature #10877

bodintsov commented Dec 30, 2024 •

edited

Loading

brianjgeiger left a comment

brianjgeiger commented Dec 30, 2024

brianjgeiger commented Dec 30, 2024

bodintsov commented Dec 30, 2024

brianjgeiger commented Dec 30, 2024

bodintsov commented Dec 30, 2024

brianjgeiger commented Jan 2, 2025

brianjgeiger left a comment

Johnetordoff commented Jan 3, 2025

bodintsov commented Jan 4, 2025 •

edited

Loading

brianjgeiger commented Jan 6, 2025

brianjgeiger commented Jan 7, 2025

Johnetordoff left a comment

Johnetordoff Jan 7, 2025

Johnetordoff Jan 7, 2025

Johnetordoff Jan 7, 2025

brianjgeiger Jan 7, 2025

Johnetordoff Jan 7, 2025

Johnetordoff Jan 7, 2025

Johnetordoff Jan 7, 2025

Johnetordoff Jan 7, 2025

		@@ -0,0 +1,18 @@
		# Generated by Django 4.2.15 on 2024-12-27 14:08

Changes for "can_request_access" feature #10877

Changes for "can_request_access" feature #10877

Conversation

bodintsov commented Dec 30, 2024 • edited Loading

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

brianjgeiger left a comment

Choose a reason for hiding this comment

brianjgeiger commented Dec 30, 2024

brianjgeiger commented Dec 30, 2024

bodintsov commented Dec 30, 2024

brianjgeiger commented Dec 30, 2024

bodintsov commented Dec 30, 2024

brianjgeiger commented Jan 2, 2025

brianjgeiger left a comment

Choose a reason for hiding this comment

Johnetordoff commented Jan 3, 2025

bodintsov commented Jan 4, 2025 • edited Loading

brianjgeiger commented Jan 6, 2025

brianjgeiger commented Jan 7, 2025

Johnetordoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodintsov commented Dec 30, 2024 •

edited

Loading

bodintsov commented Jan 4, 2025 •

edited

Loading