Skip to content

Commit 48b530a

Browse files
zzacharozzacharo
committed
fix: resolve issue with removing the last reviewer from requests
* This commit fixes a bug where removing the last reviewer from a request would cause issues in the system. The fix ensures proper handling of the reviewer removal process when no reviewers remain.
1 parent f4ffe15 commit 48b530a

File tree

7 files changed

+472
-88
lines changed

7 files changed

+472
-88
lines changed

‎invenio_requests/assets/semantic-ui/js/invenio_requests/request/reviewers/RequestReviewers.js

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -107,19 +107,15 @@ export const RequestReviewers = ({
107107
<Grid className="mt-0 mb-5">
108108
{selectedReviewers.length > 0 ? (
109109
selectedReviewers.map((reviewer) => (
110-
<>
111-
<Grid.Column width={14} className="pb-0">
112-
<React.Fragment key={reviewer.id}>
113-
{isResourceDeleted(reviewer) ? (
114-
<DeletedResource details={reviewer} />
115-
) : (
116-
<>
117-
<EntityDetails userData={reviewer} details={reviewer} />
118-
</>
119-
)}
120-
</React.Fragment>
121-
</Grid.Column>
122-
</>
110+
<Grid.Column width={14} className="pb-0" key={reviewer.id}>
111+
<React.Fragment>
112+
{isResourceDeleted(reviewer) ? (
113+
<DeletedResource details={reviewer} />
114+
) : (
115+
<EntityDetails userData={reviewer} details={reviewer} />
116+
)}
117+
</React.Fragment>
118+
</Grid.Column>
123119
))
124120
) : (
125121
<Grid.Column width={12} className="pb-0 pl-20">

‎invenio_requests/assets/semantic-ui/js/invenio_requests/request/reviewers/components/SelectedReviewersList.js

Lines changed: 28 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -25,38 +25,34 @@ export const SelectedReviewersList = ({
2525
<Header fluid as="h4" className="mb-5" size="tiny">
2626
{i18next.t(`Add up to ${maxReviewers} reviewers`)}
2727
</Header>
28-
<>
29-
{selectedReviewers.length == 0 ? (
30-
<HeaderSubheader className="pl-2 pt-2">
31-
{i18next.t("No reviewers selected")}
32-
</HeaderSubheader>
33-
) : (
34-
<Grid className="pt-10 mb-5">
35-
{selectedReviewers.map((reviewer) => (
36-
<>
37-
<Grid.Column width={13} className="pb-0">
38-
<React.Fragment key={reviewer.id}>
39-
{isResourceDeleted(reviewer) ? (
40-
<DeletedResource details={reviewer} />
41-
) : (
42-
<>
43-
<EntityDetails userData={reviewer} details={reviewer} />
44-
</>
45-
)}
46-
</React.Fragment>
47-
</Grid.Column>
48-
<Grid.Column width={2}>
49-
<Icon
50-
name="close"
51-
className="right-floated link"
52-
onClick={() => removeReviewer(reviewer.id)}
53-
/>
54-
</Grid.Column>
55-
</>
56-
))}
57-
</Grid>
58-
)}
59-
</>
28+
{selectedReviewers.length == 0 ? (
29+
<HeaderSubheader className="pl-2 pt-2">
30+
{i18next.t("No reviewers selected")}
31+
</HeaderSubheader>
32+
) : (
33+
<Grid className="pt-10 mb-5">
34+
{selectedReviewers.map((reviewer) => (
35+
<React.Fragment key={reviewer.id}>
36+
<Grid.Column width={13} className="pb-0">
37+
<React.Fragment key={reviewer.id}>
38+
{isResourceDeleted(reviewer) ? (
39+
<DeletedResource details={reviewer} />
40+
) : (
41+
<EntityDetails userData={reviewer} details={reviewer} />
42+
)}
43+
</React.Fragment>
44+
</Grid.Column>
45+
<Grid.Column width={2}>
46+
<Icon
47+
name="close"
48+
className="right-floated link"
49+
onClick={() => removeReviewer(reviewer.id)}
50+
/>
51+
</Grid.Column>
52+
</React.Fragment>
53+
))}
54+
</Grid>
55+
)}
6056
</>
6157
);
6258
};

‎invenio_requests/services/requests/components.py

Lines changed: 72 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -58,30 +58,46 @@ class RequestReviewersComponent(ServiceComponent):
5858
"""Component for handling request reviewers."""
5959

6060
def _reviewers_updated(self, previous_reviewers, new_reviewers):
61-
"""Determine the event type based on reviewers added or removed."""
62-
prev_rev = set()
63-
updated = []
64-
for reviewer in previous_reviewers:
65-
if "user" in reviewer:
66-
prev_rev.add(f"user:{reviewer['user']}")
67-
elif "group" in reviewer:
68-
prev_rev.add(f"group:{reviewer['group']}")
69-
for reviewer in new_reviewers:
70-
if "user" in reviewer:
71-
if f"user:{reviewer['user']}" not in prev_rev:
72-
updated.append(reviewer)
73-
elif "group" in reviewer:
74-
if f"group:{reviewer['group']}" not in prev_rev:
75-
updated.append(reviewer)
76-
77-
# NOTE this just supports adding OR removing at a time
78-
# if both are done, we return "updated" for now
79-
if len(previous_reviewers) > len(new_reviewers):
80-
return "removed", updated
81-
elif len(previous_reviewers) < len(new_reviewers):
82-
return "added", updated
83-
elif updated:
84-
return "updated", updated
61+
"""Determine reviewers change type: added, removed, updated, unchanged."""
62+
63+
def _normalize(reviewers):
64+
"""Convert reviewers into a set of string identifiers."""
65+
normalized = set()
66+
for r in reviewers:
67+
if "user" in r:
68+
normalized.add(f"user:{r['user']}")
69+
elif "group" in r:
70+
normalized.add(f"group:{r['group']}")
71+
return normalized
72+
73+
prev_set = _normalize(previous_reviewers)
74+
new_set = _normalize(new_reviewers)
75+
76+
added = new_set - prev_set
77+
removed = prev_set - new_set
78+
79+
if added and not removed:
80+
return "added", [
81+
r
82+
for r in new_reviewers
83+
if (
84+
("user" in r and f"user:{r['user']}" in added)
85+
or ("group" in r and f"group:{r['group']}" in added)
86+
)
87+
]
88+
elif removed and not added:
89+
return "removed", [
90+
r
91+
for r in previous_reviewers
92+
if (
93+
("user" in r and f"user:{r['user']}" in removed)
94+
or ("group" in r and f"group:{r['group']}" in removed)
95+
)
96+
]
97+
elif added and removed:
98+
return "updated", list(new_reviewers)
99+
else:
100+
return "unchanged", list(new_reviewers)
85101

86102
def _validate_reviewers(self, reviewers):
87103
"""Validate the reviewers data."""
@@ -101,27 +117,44 @@ def _validate_reviewers(self, reviewers):
101117
_(f"You can only add up to {max_reviewers} reviewers.")
102118
)
103119

104-
def update(self, identity, data=None, record=None, **kwargs):
120+
def _ensure_no_duplicates(self, reviewers):
121+
"""Ensure there are no duplicate reviewers.
122+
123+
The code is preserving the original order of reviewers.
124+
"""
125+
seen = set()
126+
unique_objs = []
127+
for d in reviewers:
128+
t = tuple(sorted(d.items()))
129+
if t not in seen:
130+
seen.add(t)
131+
unique_objs.append(d)
132+
return unique_objs
133+
134+
def update(self, identity, data=None, record=None, uow=None, **kwargs):
105135
"""Update the reviewers of a request."""
106-
if reviewers := data.get("reviewers", None):
107-
self._validate_reviewers(reviewers)
136+
if "reviewers" in data:
137+
# ensure there are not duplicates
138+
new_reviewers = self._ensure_no_duplicates(data["reviewers"])
139+
self._validate_reviewers(new_reviewers)
108140
self.service.require_permission(identity, f"action_accept", request=record)
109141

110142
event_type, updated_reviewers = self._reviewers_updated(
111-
record.get("reviewers", []), reviewers
143+
record.get("reviewers", []), new_reviewers
112144
)
113-
event = ReviewersUpdatedType(
114-
payload=dict(
115-
event="reviewers_updated",
116-
content=_(f"{event_type} a reviewer"),
117-
reviewers=updated_reviewers,
145+
if not event_type == "unchanged":
146+
event = ReviewersUpdatedType(
147+
payload=dict(
148+
event="reviewers_updated",
149+
content=_(f"{event_type} a reviewer"),
150+
reviewers=updated_reviewers,
151+
)
118152
)
119-
)
120-
_data = dict(payload=event.payload)
121-
current_events_service.create(
122-
identity, record.id, _data, event, uow=self.uow
123-
)
124-
record["reviewers"] = reviewers
153+
_data = dict(payload=event.payload)
154+
current_events_service.create(
155+
identity, record.id, _data, event, uow=uow
156+
)
157+
record["reviewers"] = new_reviewers
125158

126159

127160
class RequestPayloadComponent(DataComponent):

‎tests/conftest.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,12 @@
5454
)
5555
from marshmallow import fields
5656

57-
from invenio_requests.customizations import CommentEventType, LogEventType, RequestType
57+
from invenio_requests.customizations import (
58+
CommentEventType,
59+
LogEventType,
60+
RequestType,
61+
ReviewersUpdatedType,
62+
)
5863
from invenio_requests.notifications.builders import (
5964
CommentRequestEventCreateNotificationBuilder,
6065
)
@@ -125,6 +130,7 @@ def app_config(app_config):
125130
app_config["REQUESTS_REGISTERED_EVENT_TYPES"] = [
126131
LogEventType(),
127132
CommentEventType(),
133+
ReviewersUpdatedType(),
128134
]
129135

130136
app_config["MAIL_DEFAULT_SENDER"] = "[email protected]"
@@ -204,7 +210,7 @@ def headers():
204210
def users(UserFixture, app, database):
205211
"""Users."""
206212
users = {}
207-
for r in ["user1", "user2", "user3"]:
213+
for r in ["user1", "user2", "user3", "user4"]:
208214
u = UserFixture(
209215
email=f"{r}@example.org",
210216
password=r,

‎tests/resources/requests/conftest.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ def example_requests(app, users):
3131
"""A few example requests."""
3232
svc = app.extensions["invenio-requests"].requests_service
3333

34-
u1, u2, u3 = [u.user for u in users.values()]
34+
u1, u2, u3, _ = [u.user for u in users.values()]
3535
req1 = svc.create(
3636
sys_id, {"title": "first"}, RequestType, receiver=u1, creator=u3
3737
)._obj

0 commit comments

Comments
 (0)