[ENG-6666] NodeRequest improvements for Institutional Access Project #10826

Johnetordoff · 2024-12-05T13:50:01Z

Purpose

This feature should allow institutional admins to request a specific project and message a specific user via email with a custom message, the permission should appear as the default selected on the contributor page. This is to be done by expanding the capabilities of the NodeRequest object to allow for this behavior via the current project "Request Access" system

Changes

Split-up requests tests between node and preprint requests to keep file sizes normal
added NodeRequestTypes for a new request type INSTITUTIONAL_REQUESTS
added to_internal_value to serializer to allow ids be retrieved from RelationshipFields
added InstitutionalAdminRequestTypePermission to handle new request_type.
adds new email template
added requested_permissions to NodeRequestSerializer
adds email behavior for reply to and bccing the sender
adds unittests

QA Notes

Please make verification statements inspired by your code and what your code touches.

Verify
Verify

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

No docs for NodeRequest features.

Side Effects

Ticket

https://openscience.atlassian.net/browse/ENG-6666

Johnetordoff · 2024-12-05T22:06:06Z

api_tests/requests/views/test_node_request_list.py

@@ -0,0 +1,273 @@
+from unittest import mock


This test file was getting long and convoluted, so I split out the preprint tests.

Johnetordoff · 2024-12-05T22:08:04Z

api_tests/requests/views/test_preprint_request_list.py

@@ -0,0 +1,72 @@
+from unittest import mock


I just created this in order to prevent the test files from becoming too long.

Johnetordoff · 2024-12-05T22:09:22Z

api_tests/requests/views/test_request_list_create.py

@@ -1,189 +0,0 @@
-from unittest import mock


I split this up simply because it was getting too long and convoluted. It was difficult to isolate the correct test groups.

Johnetordoff · 2024-12-05T22:18:54Z

api/requests/serializers.py

            )
            node_request.save()
        except IntegrityError:
            raise Conflict(f'Users may not have more than one {request_type} request per node.')
-        node_request.run_submit(auth.user)
+
+        node_request.run_submit(creator)


Consider that this line already sends an email. This PR doesn't cover that that problem, but it will be changed later. This follow will use the state machine to add a curator bool the Contributor class. The follow-up will address "double emailing" for messages and deal with giving the curator role if the request is accepted.

PR for that: #10825 still wip

Johnetordoff · 2024-12-12T19:22:56Z

osf/models/request.py

@@ -22,6 +22,18 @@ class NodeRequest(AbstractRequest, NodeRequestableMixin):
    """ Request for Node Access
    """
    target = models.ForeignKey('AbstractNode', related_name='requests', on_delete=models.CASCADE)
+    request_type = models.CharField(
+        max_length=31,
+        choices=NodeRequestTypes.choices(),


RequestTypes -> NodeRequestTypes

…terForOpenScience/osf.io into institutional-access-node-request-improvements * 'feature/institutional_access' of https://github.com/CenterForOpenScience/osf.io: change to bcc the sender instead of CC-ing them revert typo add code to allow cc-ing fellow institutional admins and put their own address as a reply_to # Conflicts: # osf/migrations/0025_noderequest_requested_permissions_and_more.py

Johnetordoff · 2024-12-12T19:50:14Z

osf/utils/workflows.py

@@ -515,3 +515,9 @@ def db_name(self):
 class RequestTypes(ChoiceEnum):
    ACCESS = 'access'
    WITHDRAWAL = 'withdrawal'
+
+@unique
+class NodeRequestTypes(ChoiceEnum):


Had to split this up or I would need to suppport this for preprints.

Johnetordoff · 2024-12-12T20:32:29Z

api/requests/serializers.py

@@ -56,6 +70,8 @@ def create(self, validated_data):
        raise NotImplementedError()

 class NodeRequestSerializer(RequestSerializer):
+    request_type = ser.ChoiceField(read_only=True, required=False, choices=NodeRequestTypes.choices())


RequestTypes -> NodeRequestTypes

brianjgeiger

A few things to look at. Also, shouldn't website/templates/emails/node_request_institutional_access_request.html.mako have something in it?

api/requests/permissions.py

brianjgeiger · 2024-12-13T15:50:38Z

api/requests/permissions.py

+        try:
+            institution = Institution.objects.get(_id=institution_id)
+        except Institution.DoesNotExist:
+            return False


Is a 403 a proper response if the institution doesn't exist?

Presumably they are attempting to access a resource (the institution) that they don't have credentials for, but I see your point. I say debatable to this one, but would move it into the 400 code.

brianjgeiger · 2024-12-13T15:51:03Z

api/requests/permissions.py

            return False

+        institution_id = request.data.get('institution')
+        if not institution_id:
+            return False


Is a 403 a proper response if the institution is not in the request?

Yes this should probably be a 400, I'll make sure that's the actual response.

api/requests/serializers.py

api_tests/requests/views/test_node_request_list.py

brianjgeiger

One typo, one possible change to the email template.

api/requests/serializers.py

brianjgeiger · 2024-12-16T13:38:16Z

website/templates/emails/node_request_institutional_access_request.html.mako

+            This request is being sent to you because your project has the “Request Access” feature enabled.
+            This allows potential collaborators to request to be added to your project or to disable this feature, click
+            <a href="${node.absolute_url}/settings/">here</a>


Is this true with institutional access requests? I don't think you can disable institutional admins requesting access, can you?

Yeah I talked to Blaine about this and she indicated the users should ALWAYS be able to turn off request access, I'll double check though.

Does this mean the front end will need to know when a node has that disabled and not show the option to request for access, or will there be an email sent to the institutional admin to let them know that there request couldn't be processed because the option was turned off? Either one has consequences for the timeline, I'd be willing to bet, especially the first one.

Regardless of how the FE handles it, I've changed the check to give better info in the error, so FE can decide and added tests to demonstrate. I think that's good enough for this PR, Product designs are favoring this approach. Removing the button preemptively likely means changes to SHARE.

…e request access is turned off and added test case

Johnetordoff · 2024-12-16T17:53:33Z

api_tests/requests/views/test_node_request_list.py

@@ -4,14 +4,8 @@
 from api.base.settings.defaults import API_BASE


Just trying to keep these <500 lines.

Johnetordoff force-pushed the institutional-access-node-request-improvements branch 2 times, most recently from 54c765b to aac6fb0 Compare December 5, 2024 15:11

Johnetordoff commented Dec 5, 2024

View reviewed changes

Johnetordoff force-pushed the institutional-access-node-request-improvements branch 3 times, most recently from af18d95 to 011b708 Compare December 10, 2024 14:23

Johnetordoff changed the base branch from develop to feature/institutional_access December 12, 2024 14:55

Johnetordoff force-pushed the institutional-access-node-request-improvements branch 2 times, most recently from 30f4336 to 9a58016 Compare December 12, 2024 19:18

Johnetordoff commented Dec 12, 2024

View reviewed changes

Johnetordoff force-pushed the institutional-access-node-request-improvements branch 4 times, most recently from bc9e548 to d32cc2d Compare December 12, 2024 19:37

add NodeRequest improvements for Institutional Access

b7405af

Johnetordoff force-pushed the institutional-access-node-request-improvements branch from d32cc2d to b7405af Compare December 12, 2024 19:45

Johnetordoff commented Dec 12, 2024

View reviewed changes

fix up tests

7359973

Johnetordoff force-pushed the institutional-access-node-request-improvements branch from a209a8c to 7359973 Compare December 12, 2024 21:37

Johnetordoff marked this pull request as ready for review December 13, 2024 13:42

Johnetordoff changed the title ~~[WIP][ENG-6666] NodeRequest improvements for Institutional Access Project~~ [ENG-6666] NodeRequest improvements for Institutional Access Project Dec 13, 2024

brianjgeiger requested changes Dec 13, 2024

View reviewed changes

John Tordoff added 2 commits December 13, 2024 11:42

clean-up docstrings and typos

4d16ab5

re-introduce in email template

a024264

Johnetordoff force-pushed the institutional-access-node-request-improvements branch from 5cc90b9 to d8abf97 Compare December 13, 2024 19:24

tweak permissions for 400 error code and add test cases

e34b928

Johnetordoff force-pushed the institutional-access-node-request-improvements branch from d8abf97 to e34b928 Compare December 13, 2024 19:30

brianjgeiger requested changes Dec 16, 2024

View reviewed changes

John Tordoff added 2 commits December 16, 2024 11:30

fix typo

8b5c183

split off node request file again, added descriptive message when nod…

efc5dd2

…e request access is turned off and added test case

Johnetordoff commented Dec 16, 2024

View reviewed changes

brianjgeiger approved these changes Dec 16, 2024

View reviewed changes

Johnetordoff merged commit 66df96c into CenterForOpenScience:feature/institutional_access Dec 16, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-6666] NodeRequest improvements for Institutional Access Project #10826

[ENG-6666] NodeRequest improvements for Institutional Access Project #10826

Johnetordoff commented Dec 5, 2024 •

edited

Loading

Johnetordoff Dec 5, 2024

Johnetordoff Dec 5, 2024

Johnetordoff Dec 5, 2024

Johnetordoff Dec 5, 2024 •

edited

Loading

Johnetordoff Dec 12, 2024

Johnetordoff Dec 12, 2024

Johnetordoff Dec 12, 2024

brianjgeiger left a comment

brianjgeiger Dec 13, 2024

Johnetordoff Dec 13, 2024

brianjgeiger Dec 13, 2024

Johnetordoff Dec 13, 2024

brianjgeiger left a comment

brianjgeiger Dec 16, 2024

Johnetordoff Dec 16, 2024

brianjgeiger Dec 16, 2024

Johnetordoff Dec 16, 2024 •

edited

Loading

Johnetordoff Dec 16, 2024

		@@ -4,14 +4,8 @@
		from api.base.settings.defaults import API_BASE

[ENG-6666] NodeRequest improvements for Institutional Access Project #10826

[ENG-6666] NodeRequest improvements for Institutional Access Project #10826

Conversation

Johnetordoff commented Dec 5, 2024 • edited Loading

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johnetordoff Dec 5, 2024 • edited Loading

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

brianjgeiger 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

brianjgeiger 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

Johnetordoff Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johnetordoff commented Dec 5, 2024 •

edited

Loading

Johnetordoff Dec 5, 2024 •

edited

Loading

Johnetordoff Dec 16, 2024 •

edited

Loading