Skip to content

feat: reviewers: add users/groups as reviewers to requests #431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 1, 2025
Merged

feat: reviewers: add users/groups as reviewers to requests #431

merged 1 commit into from
Aug 1, 2025

Conversation

zzacharo
Copy link
Member

@zzacharo zzacharo commented Mar 19, 2025 •
edited by yashlamba
Loading

utnapischtim
utnapischtim reviewed Mar 19, 2025
View reviewed changes
@zzacharo zzacharo force-pushed the assign-reviewers-to-request branch 4 times, most recently from 63a31fc to 1fd7188 Compare March 27, 2025 14:24
zzacharo
zzacharo commented Apr 8, 2025
View reviewed changes
@yashlamba yashlamba mentioned this pull request Apr 10, 2025
Merged
yashlamba
yashlamba reviewed Apr 10, 2025
View reviewed changes
zzacharo
zzacharo commented Apr 10, 2025
View reviewed changes
@yashlamba yashlamba mentioned this pull request Apr 8, 2025
Closed
8 tasks
@yashlamba
Copy link
Member

yashlamba commented Apr 11, 2025 •
edited
Loading

Mostly changed spacing and alignments:

image
image
image
image

@yashlamba yashlamba force-pushed the assign-reviewers-to-request branch from 19c3e15 to c2b6788 Compare April 11, 2025 11:42
@yashlamba yashlamba changed the title wip reviewer entity reviews: adding request reviewers base implementation Apr 11, 2025
@yashlamba yashlamba marked this pull request as ready for review April 11, 2025 11:46
@yashlamba yashlamba force-pushed the assign-reviewers-to-request branch from db708dd to c462b76 Compare May 16, 2025 09:25
zzacharo
zzacharo commented Jun 5, 2025
View reviewed changes
invenio_requests/services/requests/params.py
my_requests_query = self._generate_my_requests_query(identity)
# Topic grants include requests created by the user or the user is the receiver,
# so we need to exclude them
return dsl.Q("terms", **{"grants": grants}) & ~my_requests_query
return (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: we should add this use case on the documentation regarding "what you can see on the Shared with me requests under my dashboard"

zzacharo
zzacharo commented Jun 5, 2025
View reviewed changes
zzacharo
zzacharo commented Jun 5, 2025
View reviewed changes
kpsherva
kpsherva reviewed Jun 13, 2025
View reviewed changes
invenio_requests/assets/semantic-ui/js/invenio_requests/api/InvenioRequestApi.js
@@ -73,6 +73,14 @@ export class InvenioRequestsAPI {
});
};

addReviewer = async (reviewers) => {
return await http.put(this.#urls.self, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the cancellable wrapper to avoid memory leaks if component is unmounted

kpsherva
kpsherva reviewed Jun 13, 2025
View reviewed changes
kpsherva
kpsherva reviewed Jun 13, 2025
View reviewed changes
@yashlamba yashlamba force-pushed the assign-reviewers-to-request branch from 6c94bc6 to 1f43b0f Compare July 15, 2025 12:01
yashlamba
yashlamba reviewed Jul 15, 2025
View reviewed changes
zzacharo
zzacharo commented Jul 17, 2025
View reviewed changes
invenio_requests/services/permissions.py
"accepted",
"declined",
],
[Creator(), Receiver(), Reviewers(), Topic()],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yashlamba we need to add here an IFConfig() to add reviewers to the permission only if the feature is enabled.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the IfConfig pattern is a good fit here. If you have the feature disabled, it means you can't add reviewers via the service component, which already takes the feature flag into account.

In general, we should be using the config so that:

  • the service component ignores or takes into account the passed reviewers value in create/update
  • the UI shows/hides the "Reviewers" box entirely in the request item page
    • if the feature is enabled though, the box is shown and if the user has permissions to update reviewers they should also see the searchbox so they can manage them

Permissions-based enabling/disabling of the feature leads to confusing DX and expectations, see e.g. the community membership request feature, which displays the "Request membership" button to admins, but clicking it leads to a 500 page.

zzacharo
zzacharo commented Jul 17, 2025
View reviewed changes
zzacharo
zzacharo commented Jul 17, 2025
View reviewed changes
zzacharo
zzacharo commented Jul 17, 2025
View reviewed changes
@yashlamba yashlamba changed the title reviews: adding request reviewers base implementation feat: reviewers: add users/groups as reviewers to requests Jul 30, 2025
@zzacharo zzacharo force-pushed the assign-reviewers-to-request branch from 635a289 to 7df1a4a Compare August 1, 2025 08:13
@zzacharo @yashlamba
feat: implement comprehensive reviewers functionality for requests
0f1ff6e 
This commit introduces a complete reviewers system for Invenio Requests, enabling
assignment and management of reviewers throughout the request lifecycle.

**Frontend Changes:**
- Add RequestReviewers React component with search and selection functionality
- Implement CollapsedHeader, ReviewerSearch, and SelectedReviewersList components
- Add reviewer management UI to request details page
- Integrate reviewers timeline events and API endpoints

**Backend Changes:**
- Add RequestReviewersComponent for service layer reviewer management
- Implement ReviewersUpdatedType custom event for timeline tracking
- Add reviewer-specific permissions and access control generators
- Update request schema and mappings to support reviewers field
- Add multi-entity reference support for reviewer entities

**Database & Schema:**
- Update OpenSearch mappings (v1, v2, v7) with dynamic reviewer templates
- Extend request JSON schema with reviewers definitions
- Add reviewer entity reference system fields

**Configuration & Permissions:**
- Make reviewers configurable through request types
- Add reviewer-specific permission checks and generators
- Update service configurations to handle reviewer operations

**API & Services:**
- Extend request service with reviewer update capabilities
- Add reviewer-specific API endpoints and parameters
- Implement proper validation and error handling for reviewer operations

**Testing & Quality:**
- Update test configurations and fixtures
- Fix linting issues and code style violations
- Add comprehensive test coverage for reviewer functionality

This implementation provides a complete reviewers workflow including assignment,
removal, permission management, and timeline tracking while maintaining
backward compatibility with existing request functionality.

Co-authored-by: Yash Lamba <[email protected]>
@zzacharo zzacharo force-pushed the assign-reviewers-to-request branch from 7df1a4a to 0f1ff6e Compare August 1, 2025 08:41
@zzacharo zzacharo merged commit 389b754 into inveniosoftware:master Aug 1, 2025
3 checks passed
max-moser added a commit to max-moser/invenio-requests that referenced this pull request Aug 5, 2025
@max-moser
fix(results): pass request as kwarg to permission machinery
5b1dbd3 
* partial backport of inveniosoftware#431
* previously, `has_permissions_to()` was reused from the parent class,
  which passed the request object as `record` only
* some permission generators expect the request to be available under
  the name `request` however
This was referenced Aug 5, 2025
Closed
Merged
max-moser added a commit that referenced this pull request Aug 5, 2025
@max-moser
fix(results): pass request as kwarg to permission machinery
d6d10b1 
* partial backport of #431
* previously, `has_permissions_to()` was reused from the parent class,
  which passed the request object as `record` only
* some permission generators expect the request to be available under
  the name `request` however
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@utnapischtim utnapischtim utnapischtim left review comments

@slint slint slint left review comments

@kpsherva kpsherva kpsherva left review comments

@yashlamba yashlamba yashlamba left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assign reviewers to a request
5 participants
@zzacharo @yashlamba @utnapischtim @slint @kpsherva