Feature/ENG-6134 #10951

ihorsokhanexoft · 2025-01-30T14:45:33Z

Purpose

Contributors added to a registration should be able to edit title. Admins should be able to edit contributors on registration page

Changes

Common perform_destroy for NodeContributorDetail and RegistrationContributorDetail views moved to a separate NodeContributorRemoveMixin
Added has_permission method to ContributorDetailPermissions for RegistrationList so that contributors cannot do POST request (adding contributors)
Title field of Registration is not read only right now and added in WRITABLE_WHITELIST of Registration
Added tests

Notes:

In RegistrationContributorsList.get_serializer_context I set default email = default assuming this is correct as we don't have a separate email template for registrations
I inherited from CreateModelMixin and DestroyModelMixin for some classes because didn't want to update base class as it may impact on v1 version

Ticket

https://openscience.atlassian.net/jira/software/c/projects/ENG/boards/145?assignee=712020%3A7c7368dc-40cb-475f-bae8-b07a8bd2dd6c&selectedIssue=ENG-6134

…adata page

…tration

…unctionality

…ibutor

Johnetordoff

Very good tests and diligent work, I have one suggestion about class inheritance, but otherwise good.

Also I see some unit test failures. Fix those before putting it back in review.

api/nodes/permissions.py

Johnetordoff · 2025-01-30T17:29:57Z

api/nodes/views.py

@@ -361,6 +361,19 @@ def perform_destroy(self, instance):
            raise PermissionDenied(str(err))


+class NodeContributorRemoveMixin:


Don't add this add a Mixin just over write for the classes you want individually.

Johnetordoff · 2025-01-30T17:31:30Z

api/nodes/views.py

+    def perform_destroy(self, instance):
+        node = self.get_resource()
+        auth = get_user_auth(self.request)
+        if len(node.visible_contributors) == 1 and instance.visible:


use node.visible_contributors.count()

yeah, just copied this block from another place ;)

It is okay to copy, I copy a lot, but only from good developers! :)

Johnetordoff · 2025-01-30T17:33:04Z

api/nodes/views.py

@@ -521,16 +534,6 @@ class NodeContributorDetail(BaseContributorDetail, generics.RetrieveUpdateDestro
    def get_resource(self):
        return self.get_node()

-    # overrides DestroyAPIView
-    def perform_destroy(self, instance):


As I said above don't bother using a mixin, it's okay to repeat yourself a little across views. It's just easier to see new behavior when you have a small number of methods for a view.

Johnetordoff · 2025-01-30T17:37:22Z

api_tests/registrations/views/test_registration_list.py

+                }
+            }
+        }
+        payload = {'data': {'attributes': attrs, 'relationships': relationships, 'type': 'contributors'}}


just a small thing but I would like everything to be indented like the relationships dictionary.

Johnetordoff · 2025-01-30T17:41:35Z

api/registrations/views.py

@@ -267,7 +268,7 @@ def get_serializer_context(self):
        return context


-class RegistrationContributorsList(BaseContributorList, RegistrationMixin, UserMixin):
+class RegistrationContributorsList(BaseContributorList, mixins.CreateModelMixin, RegistrationMixin, UserMixin):


This is a good use of mixin. this class looks correct.

brianjgeiger

Just a couple of things from me, plus handle the failing tests please.

api/registrations/serializers.py

brianjgeiger · 2025-01-30T15:18:41Z

api_tests/registrations/views/test_registration_list.py

+            creator=self.user, project=self.project)
+
+        self.public_project = ProjectFactory(is_public=True, creator=self.user)
+        self.public_registration_project = RegistrationFactory(


Minor: I would call this public_registration instead of public_registration_project

yeah, agree, just copied from another place

ihorsokhanexoft · 2025-01-31T15:50:04Z

Fixed flake8 related errors and will fix others as you review it again because some of them don't fail locally. I assume most of the errors are related to the functionality that was prohibited (e.g. title was unchangable, impossible to update contributors of the registrations, etc) but now is allowed

brianjgeiger · 2025-01-31T20:33:12Z

@ihorsokhanexoft Looks like you still have a Flake8 problem, and at least one more failing test.

…ializer_context, fixed flake and tests where title is not writable

ihorsokhanexoft · 2025-02-03T08:13:53Z

@brianjgeiger pushed the last fix. Review please and I'll check the result of tests

brianjgeiger · 2025-02-03T14:16:14Z

@ihorsokhanexoft Looks like we're getting about 75 failing tests with a 400 response rather than successful responses. Hopefully there's just a small change to fix all those.

brianjgeiger · 2025-02-03T14:19:09Z

If the failing tests are because we're requiring the Title field, then I may have been wrong about that and we can make it not required again.

ihorsokhanexoft · 2025-02-03T14:33:33Z

If the failing tests are because we're requiring the Title field, then I may have been wrong about that and we can make it not required again.

@brianjgeiger Yeah, it's because this field is required now. Reverted this change

ihorsokhanexoft · 2025-02-03T15:12:42Z

@brianjgeiger 1 test failed because it expected title shouldn't be updated. fixed it

brianjgeiger

One more bit of functionalist that appears to be missing. Almost there.

brianjgeiger · 2025-02-03T16:09:00Z

api_tests/registrations/views/test_registration_list.py

+        url = f'/{API_BASE}registrations/{self.public_registration._id}/contributors/{contributor._id}/'
+        return self.app.delete_json_api(url, auth=auth_user.auth, expect_errors=expect_errors)
+
+    def update_attribute_request(self, auth_user, expect_errors=True, **attributes):


Sorry I missed this before. Admins should also be able to modify the contributors permissions and bibliographic status with this PR.

I see, it means we need to add patch method to RegistrationContributorDetail view

…sions for registration

ihorsokhanexoft · 2025-02-04T13:27:36Z

@brianjgeiger @Johnetordoff I've found update method in the last commit in NodeContributorDetailSerializer that is used for draft registrations update, so I reused it

brianjgeiger · 2025-02-04T13:42:46Z

Looks like there's at least one problem that needs fixing from CI: api_tests/registrations/views/test_registration_list.py:2094:64: E712 comparison to True should be 'if cond is True:' or 'if cond:'

ihorsokhanexoft · 2025-02-04T14:12:05Z

api_tests/registrations/views/test_registration_list.py:2094:64

Yeah, it's quite bad that I can't check the errors before your review. Fixed

brianjgeiger

I think one last thing, to DRY up the update method.

brianjgeiger · 2025-02-04T19:36:34Z

api/registrations/serializers.py

+        index = None
+        if '_order' in validated_data:
+            index = validated_data.pop('_order')
+
+        auth = Auth(self.context['request'].user)
+        node = self.context['resource']
+
+        if 'bibliographic' in validated_data:
+            bibliographic = validated_data.get('bibliographic')
+        else:
+            bibliographic = node.get_visible(instance.user)
+        permission = validated_data.get('permission') or instance.permission
+        try:
+            if index is not None:
+                node.move_contributor(instance.user, auth, index, save=True)
+            node.update_contributor(instance.user, permission, bibliographic, auth, save=True)
+        except node.state_error as e:
+            raise exceptions.ValidationError(detail=str(e))
+        except ValueError as e:
+            raise exceptions.ValidationError(detail=str(e))
+        instance.refresh_from_db()
+        return instance


Could you extract this into a helper function and call that function from both places it's used? It's significant enough functionality that it would be nice to have it DRY. Maybe put it in api/base/utils.py

Agree. Followed John's recommendations it's not always required to follow DRY and no need to create a separate mixin

brianjgeiger · 2025-02-05T14:14:51Z

Looks like you're missing some commas according to the pre-commit hook.

index f9fc0cc..bef17a7 100644
--- a/api/nodes/serializers.py
+++ b/api/nodes/serializers.py
@@ -18,7 +18,7 @@ from api.base.settings import ADDONS_FOLDER_CONFIGURABLE
 from api.base.utils import (
     absolute_reverse, get_object_or_error,
     get_user_auth, is_truthy,
-    update_contributors_permissions_and_bibliographic_status
+    update_contributors_permissions_and_bibliographic_status,
 )
 from api.base.versioning import get_kebab_snake_case_field
 from api.institutions.utils import update_institutions
@@ -1272,7 +1272,7 @@ class NodeContributorDetailSerializer(NodeContributorsSerializer):
         return update_contributors_permissions_and_bibliographic_status(
             self,
             instance,
-            validated_data
+            validated_data,
         )
 
 
diff --git a/api/registrations/serializers.py b/api/registrations/serializers.py
index 2f3ded4..0e35421 100644
--- a/api/registrations/serializers.py
+++ b/api/registrations/serializers.py
@@ -923,7 +923,7 @@ class RegistrationContributorsSerializer(NodeContributorsSerializer):
         return update_contributors_permissions_and_bibliographic_status(
             self,
             instance,
-            validated_data
+            validated_data,
         )

It might be worth automatically getting pre-commit hooks running for you locally.

ihorsokhanexoft · 2025-02-05T14:49:23Z

It might be worth automatically getting pre-commit hooks running for you locally.

Yes, I have installed it, but it doesn't show any warnings, all checks are passed even if I run it manually

ihorsokhanexoft added 6 commits January 27, 2025 13:13

added an ability to update title for registration on overview and met…

177888d

…adata page

added an ability to add contributor to registration

b9d1475

added an ability to remove contributor and his permissions from regis…

78f9e7b

…tration

fixed inheritance ordering

6045131

added happy path tests for add/remove contributor for registrations f…

90e2af1

…unctionality

added more test cases and fixed permission when user adds a new contr…

8c73a3d

…ibutor

brianjgeiger changed the base branch from develop to feature/b-and-i-25-01 January 30, 2025 15:03

Johnetordoff reviewed Jan 30, 2025

View reviewed changes

brianjgeiger requested changes Jan 30, 2025

View reviewed changes

ihorsokhanexoft added 2 commits January 31, 2025 17:17

minor changes for reviewers

c407b1c

flake8 improvements

cc99aa2

ihorsokhanexoft requested review from Johnetordoff and brianjgeiger January 31, 2025 15:47

fixed title for POST request, removed redundant parameter for get_ser…

63179a1

…ializer_context, fixed flake and tests where title is not writable

revert title required=True

1c97bbd

fixed a test with title update

6531034

brianjgeiger requested changes Feb 3, 2025

View reviewed changes

added ability to modify bibliographic status and contributors' permis…

a29540f

…sions for registration

fixed flake8 comparison

c046b55

ihorsokhanexoft requested a review from brianjgeiger February 4, 2025 14:19

fixed flake8

14db30b

brianjgeiger requested changes Feb 4, 2025

View reviewed changes

follow DRY principle

7fb7d26

brianjgeiger approved these changes Feb 5, 2025

View reviewed changes

flake8 fix

4ce2216

brianjgeiger merged commit d370e2b into CenterForOpenScience:feature/b-and-i-25-01 Feb 5, 2025
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/ENG-6134 #10951

Feature/ENG-6134 #10951

ihorsokhanexoft commented Jan 30, 2025

Johnetordoff left a comment •

edited

Loading

Johnetordoff Jan 30, 2025

Johnetordoff Jan 30, 2025

ihorsokhanexoft Jan 31, 2025

Johnetordoff Jan 31, 2025

Johnetordoff Jan 30, 2025

Johnetordoff Jan 30, 2025

Johnetordoff Jan 30, 2025

brianjgeiger left a comment

brianjgeiger Jan 30, 2025

ihorsokhanexoft Jan 31, 2025

ihorsokhanexoft commented Jan 31, 2025 •

edited

Loading

brianjgeiger commented Jan 31, 2025

ihorsokhanexoft commented Feb 3, 2025

brianjgeiger commented Feb 3, 2025

brianjgeiger commented Feb 3, 2025

ihorsokhanexoft commented Feb 3, 2025

ihorsokhanexoft commented Feb 3, 2025

brianjgeiger left a comment

brianjgeiger Feb 3, 2025

ihorsokhanexoft Feb 4, 2025 •

edited

Loading

ihorsokhanexoft commented Feb 4, 2025 •

edited

Loading

brianjgeiger commented Feb 4, 2025

ihorsokhanexoft commented Feb 4, 2025

brianjgeiger left a comment

brianjgeiger Feb 4, 2025

ihorsokhanexoft Feb 5, 2025

brianjgeiger commented Feb 5, 2025

ihorsokhanexoft commented Feb 5, 2025 •

edited

Loading

		@@ -361,6 +361,19 @@ def perform_destroy(self, instance):
		raise PermissionDenied(str(err))


		class NodeContributorRemoveMixin:

Feature/ENG-6134 #10951

Feature/ENG-6134 #10951

Conversation

ihorsokhanexoft commented Jan 30, 2025

Purpose

Changes

Notes:

Ticket

Johnetordoff left a comment • 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

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

ihorsokhanexoft commented Jan 31, 2025 • edited Loading

brianjgeiger commented Jan 31, 2025

ihorsokhanexoft commented Feb 3, 2025

brianjgeiger commented Feb 3, 2025

brianjgeiger commented Feb 3, 2025

ihorsokhanexoft commented Feb 3, 2025

ihorsokhanexoft commented Feb 3, 2025

brianjgeiger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihorsokhanexoft Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

ihorsokhanexoft commented Feb 4, 2025 • edited Loading

brianjgeiger commented Feb 4, 2025

ihorsokhanexoft commented Feb 4, 2025

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

brianjgeiger commented Feb 5, 2025

ihorsokhanexoft commented Feb 5, 2025 • edited Loading

Johnetordoff left a comment •

edited

Loading

ihorsokhanexoft commented Jan 31, 2025 •

edited

Loading

ihorsokhanexoft Feb 4, 2025 •

edited

Loading

ihorsokhanexoft commented Feb 4, 2025 •

edited

Loading

ihorsokhanexoft commented Feb 5, 2025 •

edited

Loading