[ENG-6668] Add Contributor Page Improvements for Institutional Access #10825

Johnetordoff · 2024-12-05T13:34:56Z

Purpose

The new Institutional Access feature creates a new "Curator" role that has certain requirements. The first is that it have a special label in the contributor list, shown here:

The second requirement is that the user is unable to set the curator to a bibliographic or "visible contributor.

Changes

Add check constraint to avoid visible institutional admins.
Locates area in page where Curator status can be lableled
adds tests to test for specific behavior at model and API level
adds extra UI notice for contributor page.

QA Notes

What are the areas of risk?

Any concerns/considerations/questions that development raised?

Documentation

Side Effects

Ticket

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

Johnetordoff · 2024-12-05T17:58:52Z

osf/models/node.py

@@ -1079,7 +1079,13 @@ def set_visible(self, user, visible, log=True, auth=None, save=False):
        if not self.is_contributor(user):
            raise ValueError(f'User {user} not in contributors')
        if visible and not Contributor.objects.filter(node=self, user=user, visible=True).exists():
-            Contributor.objects.filter(node=self, user=user, visible=False).update(visible=True)


Overwriting save doesn't always cut it, but this looks like that only place we .update visibility, arguably we want to retain the ability to change the visibility which leans away from placing in DB constraints. However I think this is all visibility updates in the existing code.

…or.mako and make boolean is_curator field

brianjgeiger

A few comments and questions.

osf/migrations/0025_contributor_is_curator_and_more.py

osf/models/contributor.py

osf_tests/test_institutional_admin_contributors.py

website/static/js/accessRequestManager.js

osf/models/user.py

…nals

…dation

…methods and add disabled checkboxes to UI

brianjgeiger

One tiny thing, and then one more check to see if we're using the right information for making UI decisions.

brianjgeiger · 2025-01-10T16:05:50Z

website/profile/utils.py

@@ -33,6 +33,8 @@ def serialize_user(user, node=None, admin=False, full=False, is_profile=False, i
        'surname': user.family_name,
        'fullname': fullname,
        'shortname': fullname if len(fullname) < 50 else fullname[:23] + '...' + fullname[-23:],
+        'is_institutional_admin': user.is_institutional_admin(),


Do we need this for this view? It looks like it's being used, but is it being used properly? See below for more.

brianjgeiger · 2025-01-10T16:06:22Z

website/project/views/contributor.py

@@ -287,6 +288,13 @@ def project_manage_contributors(auth, node, **kwargs):
        node.manage_contributors(contributors, auth=auth, save=True)
    except (ValueError, NodeStateError) as error:
        raise HTTPError(http_status.HTTP_400_BAD_REQUEST, data={'message_long': error.args[0]})
+    except IntegrityError as error:
+        status.push_status_message(
+            'You can not make an institutional admin a bibliographic contributor.',


Suggested change

'You can not make an institutional admin a bibliographic contributor.',

'You can not make an institutional curator a bibliographic contributor.',

brianjgeiger · 2025-01-10T16:12:52Z

website/templates/project/contributors.mako

@@ -366,11 +406,26 @@
        <td>
            <div class="header" data-bind="visible: accessRequest.expanded()  && $root.collapsed()"></div>
            <div class="td-content" data-bind="visible: !$root.collapsed() || accessRequest.expanded()">
+            <div data-bind="ifnot: accessRequest.user.is_institutional_admin">


Is this correct, or is it that the request was made from the institutional dashboard? Does the request need to store this info, rather than inferring it from the institutional admin status? This goes back to not wanting all institutional admins to be automatically classified as a curator request.

Yes you are correct, I checked with Product again and they confirmed. This should ensure the user actually has made a pending institutional access request, not a normal access request.

…uests, fix tpyos

brianjgeiger

Almost there. One optimization left.

brianjgeiger · 2025-01-10T18:10:13Z

osf/models/user.py

+        return NodeRequest.objects.filter(
+            request_type=NodeRequestTypes.INSTITUTIONAL_REQUEST.value,
+            target=node,
+            creator=self,
+        ).exists()


Just to be sure, is there any place we can cache this info, such as a request object, rather than relying on this query, the same way we did with caching on the contributor object if it's a curator? Same reason as before, to avoid making this query for every contributor that gets serialized.

brianjgeiger · 2025-01-10T18:12:26Z

website/templates/project/contributors.mako

@@ -406,23 +405,23 @@
        <td>
            <div class="header" data-bind="visible: accessRequest.expanded()  && $root.collapsed()"></div>
            <div class="td-content" data-bind="visible: !$root.collapsed() || accessRequest.expanded()">
-            <div data-bind="ifnot: accessRequest.user.is_institutional_admin">
+            <div data-bind="ifnot: accessRequest.user.has_institutional_request">


So here, for example, could be accessRequest.is_curator or similar.

brianjgeiger

Still a bit more. What you did was not what I was asking for.

brianjgeiger · 2025-01-10T18:36:30Z

osf/models/contributor.py

+        """
+        Checks if user a has requested a node using the institutional access request feature.
+        """
+        return self.user.has_institutional_request(self.node)


This is still going to make the query once per contributor when loading the page. Could we make a boolean field on the NodeRequest (or maybe AbstractRequest, if we want to plan for the future) model to hold this information?

…ializing more directly from the objects to the template

brianjgeiger

🎉 🐧 🎉

brianjgeiger · 2025-01-10T21:17:43Z

Well, except for the failing tests.

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch 3 times, most recently from e61b288 to e532dae Compare December 5, 2024 17:53

Johnetordoff commented Dec 5, 2024

View reviewed changes

Johnetordoff mentioned this pull request Dec 12, 2024

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

Merged

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

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch 4 times, most recently from 456fde5 to 9c307a7 Compare January 7, 2025 21:05

Johnetordoff changed the title ~~[WIP][ENG-6668] Add Contributor Page Improvements for Institutional Access~~ [ENG-6668] Add Contributor Page Improvements for Institutional Access Jan 7, 2025

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch from adcdd3f to 70f2629 Compare January 7, 2025 21:42

Johnetordoff changed the title ~~[ENG-6668] Add Contributor Page Improvements for Institutional Access~~ [WIP][ENG-6668] Add Contributor Page Improvements for Institutional Access Jan 7, 2025

Johnetordoff marked this pull request as ready for review January 8, 2025 14:30

Johnetordoff requested a review from brianjgeiger January 8, 2025 14:30

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch from 70f2629 to 9bd37a2 Compare January 8, 2025 14:47

Johnetordoff removed the request for review from brianjgeiger January 8, 2025 14:47

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch 10 times, most recently from 56f0959 to 9a72225 Compare January 9, 2025 18:47

Johnetordoff changed the title ~~[WIP][ENG-6668] Add Contributor Page Improvements for Institutional Access~~ [ENG-6668] Add Contributor Page Improvements for Institutional Access Jan 9, 2025

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch from cd1629d to 2ea118e Compare January 9, 2025 19:24

validate curator's non-biblio status and display details on contrbut…

4e507b1

…or.mako and make boolean is_curator field

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch from 2ea118e to 4e507b1 Compare January 9, 2025 20:20

brianjgeiger requested changes Jan 9, 2025

View reviewed changes

John Tordoff added 4 commits January 9, 2025 16:23

add is _curator in it's own migration file

52d1a43

refactor and split up permissions checks and improve .save conditio…

d1a7412

…nals

add positive test cases for institutional access project curator vali…

5c33833

…dation

redo do contributor.mako logic to work for new more specific curator …

756a53d

…methods and add disabled checkboxes to UI

brianjgeiger requested changes Jan 10, 2025

View reviewed changes

ensure insti admins are only labeled as such when they make insti req…

8ef8cd6

…uests, fix tpyos

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch from f7dfae4 to 8ef8cd6 Compare January 10, 2025 17:02

brianjgeiger requested changes Jan 10, 2025

View reviewed changes

revert last commit and fix data model by adding more booleans and ser…

0da606a

…ializing more directly from the objects to the template

Johnetordoff force-pushed the institutional-access-insti-admin-validation branch from 0dc2738 to 0da606a Compare January 10, 2025 20:44

brianjgeiger approved these changes Jan 10, 2025

View reviewed changes

John Tordoff added 2 commits January 10, 2025 16:37

fix update contributor check

2bbcf07

fix conditional for is_curator check in contributor management method

6c8eb0a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENG-6668] Add Contributor Page Improvements for Institutional Access #10825

[ENG-6668] Add Contributor Page Improvements for Institutional Access #10825

Johnetordoff commented Dec 5, 2024 •

edited

Loading

Johnetordoff Dec 5, 2024 •

edited

Loading

brianjgeiger left a comment

brianjgeiger left a comment

brianjgeiger Jan 10, 2025

brianjgeiger Jan 10, 2025

brianjgeiger Jan 10, 2025

Johnetordoff Jan 10, 2025

brianjgeiger left a comment

brianjgeiger Jan 10, 2025

brianjgeiger Jan 10, 2025

brianjgeiger left a comment

brianjgeiger Jan 10, 2025

brianjgeiger left a comment

brianjgeiger commented Jan 10, 2025

	'You can not make an institutional admin a bibliographic contributor.',
	'You can not make an institutional curator a bibliographic contributor.',

[ENG-6668] Add Contributor Page Improvements for Institutional Access #10825

Are you sure you want to change the base?

[ENG-6668] Add Contributor Page Improvements for Institutional Access #10825

Conversation

Johnetordoff commented Dec 5, 2024 • edited Loading

Purpose

Changes

QA Notes

Documentation

Side Effects

Ticket

Johnetordoff Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

brianjgeiger Jan 10, 2025

Choose a reason for hiding this comment

brianjgeiger Jan 10, 2025

Choose a reason for hiding this comment

brianjgeiger Jan 10, 2025

Choose a reason for hiding this comment

Johnetordoff Jan 10, 2025

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

brianjgeiger Jan 10, 2025

Choose a reason for hiding this comment

brianjgeiger Jan 10, 2025

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

brianjgeiger Jan 10, 2025

Choose a reason for hiding this comment

brianjgeiger left a comment

Choose a reason for hiding this comment

brianjgeiger commented Jan 10, 2025

Johnetordoff commented Dec 5, 2024 •

edited

Loading

Johnetordoff Dec 5, 2024 •

edited

Loading