Feature/review patch by danieleguido · Pull Request #667 · impresso/impresso-middle-layer

danieleguido · 2026-02-13T07:41:47Z

No description provided.

Injects CeleryClient into the UserSpecialMembershipRequestReviewsService and implements a strict single-item patch method that verifies reviewer authorization (direct reviewer or via special membership access), updates the request, and enqueues an after-update Celery task. Type imports and the service interface were adjusted to include NullableId and an explicit patch signature. Unit tests were extended to use a Redis/Celery test setup (setupTestDatabaseRedisCelery), track Celery/Redis calls, and cover patch scenarios (direct reviewer, special-access reviewer, unauthorized reviewer, missing id, and non-existent request).

Introduce setupTestDatabaseRedisCelery to create an in-memory test DB and provide mocked Celery and Redis clients. The new helper accepts arrays/records to capture celery run calls and redis setEx calls, patches the test app's get and service methods to return the mocked clients, and exposes utilities for get/get/del on the mocked Redis client. Also add related types and imports to support the mocks, enabling tests to assert on background task and cache interactions.

Copilot

Pull request overview

This PR adds a patch method to the UserSpecialMembershipRequestReviewsService to allow reviewers to update the status of special membership requests. The implementation includes authorization checks to ensure only assigned reviewers can modify requests, integrates with Celery for async notifications, and adds comprehensive test coverage.

Changes:

Added patch method to enable reviewers to update request status (approved/rejected)
Created setupTestDatabaseRedisCelery helper function to support testing with mocked Celery and Redis clients
Added five test cases covering authorization, success paths, and error scenarios

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`src/services/user-special-membership-requests-reviews/user-special-membership-requests-reviews.class.ts`	Implements patch method with reviewer authorization checks and Celery integration for status updates
`test/helpers/database.ts`	Adds new test database setup function with mocked Celery and Redis clients for tracking async operations
`test/unit/services/user-special-membership-requests-reviews.test.ts`	Adds comprehensive test suite for patch method covering direct reviewers, special access reviewers, authorization failures, and edge cases

Copilot · 2026-02-13T07:46:14Z

...s/user-special-membership-requests-reviews/user-special-membership-requests-reviews.class.ts

+    if (this.celeryClient)
+      this.celeryClient
+        .run({
+          task: 'impresso.tasks.userSpecialMembershipRequest_tasks.after_special_membership_request_updated',
+          args: [record.id],
+        })
+        .catch(err => {
+          console.error('Error sending after_special_membership_request_updated task:', err)
+        })


The Celery client call is not awaited, which means the test assertion on line 340 may execute before the celeryClient.run() call completes. This creates a race condition where the test might pass or fail unpredictably. Looking at similar code in the terms-of-use service (src/services/terms-of-use/terms-of-use.class.ts:87), the celeryClient.run() call is properly awaited. Consider either awaiting the call directly or ensuring the promise resolves before returning from the patch method.

Suggested change

if (this.celeryClient)

this.celeryClient

.run({

task: 'impresso.tasks.userSpecialMembershipRequest_tasks.after_special_membership_request_updated',

args: [record.id],

})

.catch(err => {

console.error('Error sending after_special_membership_request_updated task:', err)

})

if (this.celeryClient) {

try {

await this.celeryClient.run({

task: 'impresso.tasks.userSpecialMembershipRequest_tasks.after_special_membership_request_updated',

args: [record.id],

})

} catch (err) {

console.error('Error sending after_special_membership_request_updated task:', err)

}

}

Copilot · 2026-02-13T07:46:14Z

...s/user-special-membership-requests-reviews/user-special-membership-requests-reviews.class.ts

+      throw new NotFound(`UserSpecialMembershipRequest with id ${id} not found`)
+    }
+
+    await record.update(data)


The patch method should update the dateLastModified field when modifying the request, similar to how it's set during creation in the user-special-membership-requests service (line 81 of user-special-membership-requests.class.ts). This ensures proper tracking of when requests are last modified and maintains consistency with the data model expectations.

Suggested change

await record.update(data)

const updateData = { ...(data || {}), dateLastModified: new Date() }

await record.update(updateData)

Copilot · 2026-02-13T07:46:14Z

...s/user-special-membership-requests-reviews/user-special-membership-requests-reviews.class.ts

+  async patch(
+    id: Id,
+    data: Partial<UserSpecialMembershipRequestModel>,
+    params?: UserSpecialMembershipRequestParams
+  ): Promise<UserSpecialMembershipRequestModel> {
+    const reviewerId = params?.user?.id
+    debug('Patching request %s by reviewer %s', id, reviewerId)
+    if (id == null) {
+      throw new NotFound('UserSpecialMembershipRequest id is required')
+    }
+    const record = await this.requestModel.findByPk(id, {
+      include: ['specialMembershipAccess'],
+    })
+    const isDirectReviewer = record && record.reviewerId === reviewerId
+    const isSpecialAccessReviewer =
+      record &&
+      (record as any).specialMembershipAccess &&
+      (record as any).specialMembershipAccess.reviewerId === reviewerId
+
+    if (!record || (!isDirectReviewer && !isSpecialAccessReviewer)) {
+      throw new NotFound(`UserSpecialMembershipRequest with id ${id} not found`)
+    }
+
+    await record.update(data)


The patch method lacks input validation for the status field and other data fields. The status should be validated against the available statuses ('pending', 'approved', 'rejected') defined in the model, similar to how the find method validates status in the service hooks. Without validation, invalid status values could be written to the database. Consider adding a validation hook in the service configuration file or validating the input data within the patch method itself.

Copilot · 2026-02-13T07:46:14Z

...s/user-special-membership-requests-reviews/user-special-membership-requests-reviews.class.ts

+          args: [record.id],
+        })
+        .catch(err => {
+          console.error('Error sending after_special_membership_request_updated task:', err)


Use the Winston logger instead of console.error for consistency with the rest of the codebase. The logger should be imported from '@/logger.js' and used as 'logger.error()'. This ensures consistent log formatting and enables proper log level management throughout the application.

Copilot · 2026-02-13T07:46:15Z

test/unit/services/user-special-membership-requests-reviews.test.ts

-import type { TestDatabase } from '../../helpers/database.js'
-import { setupTestDatabase, teardownTestDatabase } from '../../helpers/database.js'
+import type { CeleryCall, RedisSetExCall, TestDatabase } from '../../helpers/database.js'
+import { setupTestDatabase, setupTestDatabaseRedisCelery, teardownTestDatabase } from '../../helpers/database.js'


Unused import setupTestDatabase.

Suggested change

import { setupTestDatabase, setupTestDatabaseRedisCelery, teardownTestDatabase } from '../../helpers/database.js'

import { setupTestDatabaseRedisCelery, teardownTestDatabase } from '../../helpers/database.js'

danieleguido · 2026-02-13T16:08:07Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-02-13T16:08:15Z

@danieleguido I've opened a new pull request, #668, to work on those changes. Once the pull request is ready, I'll request review from you.

…est patch method (#668) * Initial plan * Address review comments: await Celery call, validate status, update dateLastModified, use logger Co-authored-by: danieleguido <[email protected]> * Simplify spread operator in patch method Co-authored-by: danieleguido <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: danieleguido <[email protected]>

danieleguido added 2 commits February 13, 2026 08:41

danieleguido requested a review from Copilot February 13, 2026 07:41

Copilot started reviewing on behalf of danieleguido February 13, 2026 07:42 View session

Copilot AI reviewed Feb 13, 2026

View reviewed changes

Copilot AI mentioned this pull request Feb 13, 2026

Fix race condition and add validation in user special membership request patch method #668

Merged

Copilot AI and others added 2 commits February 15, 2026 17:43

Merge branch 'develop' into feature/review-patch

9d6988e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Feature/review patch#667

Feature/review patch#667
danieleguido wants to merge 4 commits intodevelopfrom
feature/review-patch

danieleguido commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

Copilot AI Feb 13, 2026

Uh oh!

danieleguido commented Feb 13, 2026

Uh oh!

Copilot AI commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	await record.update(data)
	const updateData = { ...(data \|\| {}), dateLastModified: new Date() }
	await record.update(updateData)

	import { setupTestDatabase, setupTestDatabaseRedisCelery, teardownTestDatabase } from '../../helpers/database.js'
	import { setupTestDatabaseRedisCelery, teardownTestDatabase } from '../../helpers/database.js'

Comments

Conversation

danieleguido commented Feb 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

danieleguido commented Feb 13, 2026

Uh oh!

Copilot AI commented Feb 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants