diff --git a/src/db/file/pushes.ts b/src/db/file/pushes.ts index 416845688..c0827a7e0 100644 --- a/src/db/file/pushes.ts +++ b/src/db/file/pushes.ts @@ -111,7 +111,7 @@ export const authorise = async (id: string, attestation: any): Promise<{ message return { message: `authorised ${id}` }; }; -export const reject = async (id: string, attestation: any): Promise<{ message: string }> => { +export const reject = async (id: string, rejection: any): Promise<{ message: string }> => { const action = await getPush(id); if (!action) { throw new Error(`push ${id} not found`); @@ -120,7 +120,7 @@ export const reject = async (id: string, attestation: any): Promise<{ message: s action.authorised = false; action.canceled = false; action.rejected = true; - action.attestation = attestation; + action.rejection = rejection; await writeAudit(action); return { message: `reject ${id}` }; }; diff --git a/src/db/index.ts b/src/db/index.ts index f71179cf3..50d877922 100644 --- a/src/db/index.ts +++ b/src/db/index.ts @@ -167,8 +167,8 @@ export const deletePush = (id: string): Promise => start().deletePush(id); export const authorise = (id: string, attestation: any): Promise<{ message: string }> => start().authorise(id, attestation); export const cancel = (id: string): Promise<{ message: string }> => start().cancel(id); -export const reject = (id: string, attestation: any): Promise<{ message: string }> => - start().reject(id, attestation); +export const reject = (id: string, rejection: any): Promise<{ message: string }> => + start().reject(id, rejection); export const getRepos = (query?: Partial): Promise => start().getRepos(query); export const getRepo = (name: string): Promise => start().getRepo(name); export const getRepoByUrl = (url: string): Promise => start().getRepoByUrl(url); diff --git a/src/db/mongo/pushes.ts b/src/db/mongo/pushes.ts index 968b2858a..4ecb1659e 100644 --- a/src/db/mongo/pushes.ts +++ b/src/db/mongo/pushes.ts @@ -77,7 +77,7 @@ export const authorise = async (id: string, attestation: any): Promise<{ message return { message: `authorised ${id}` }; }; -export const reject = async (id: string, attestation: any): Promise<{ message: string }> => { +export const reject = async (id: string, rejection: any): Promise<{ message: string }> => { const action = await getPush(id); if (!action) { throw new Error(`push ${id} not found`); @@ -85,7 +85,7 @@ export const reject = async (id: string, attestation: any): Promise<{ message: s action.authorised = false; action.canceled = false; action.rejected = true; - action.attestation = attestation; + action.rejection = rejection; await writeAudit(action); return { message: `reject ${id}` }; }; diff --git a/src/db/types.ts b/src/db/types.ts index e4ae2eab5..e43aff295 100644 --- a/src/db/types.ts +++ b/src/db/types.ts @@ -98,7 +98,7 @@ export interface Sink { deletePush: (id: string) => Promise; authorise: (id: string, attestation: any) => Promise<{ message: string }>; cancel: (id: string) => Promise<{ message: string }>; - reject: (id: string, attestation: any) => Promise<{ message: string }>; + reject: (id: string, rejection: any) => Promise<{ message: string }>; getRepos: (query?: Partial) => Promise; getRepo: (name: string) => Promise; getRepoByUrl: (url: string) => Promise; diff --git a/src/proxy/actions/Action.ts b/src/proxy/actions/Action.ts index d9ea96feb..8c6303e9c 100644 --- a/src/proxy/actions/Action.ts +++ b/src/proxy/actions/Action.ts @@ -1,6 +1,6 @@ import { processGitURLForNameAndOrg, processUrlPath } from '../routes/helper'; import { Step } from './Step'; -import { Attestation, CommitData } from '../processors/types'; +import { Attestation, CommitData, Rejection } from '../processors/types'; /** * Class representing a Push. @@ -34,6 +34,7 @@ class Action { user?: string; userEmail?: string; attestation?: Attestation; + rejection?: Rejection; lastStep?: Step; proxyGitPath?: string; newIdxFiles?: string[]; diff --git a/src/proxy/processors/types.ts b/src/proxy/processors/types.ts index c4c447b5d..aa5c23fea 100644 --- a/src/proxy/processors/types.ts +++ b/src/proxy/processors/types.ts @@ -19,6 +19,15 @@ export type Attestation = { questions: Question[]; }; +export type Rejection = { + reviewer: { + username: string; + reviewerEmail: string; + }; + timestamp: string | Date; + reason: string; +}; + export type CommitContent = { item: number; type: number; diff --git a/src/service/routes/push.ts b/src/service/routes/push.ts index fbce5335e..4fff08769 100644 --- a/src/service/routes/push.ts +++ b/src/service/routes/push.ts @@ -45,6 +45,14 @@ router.post('/:id/reject', async (req: Request, res: Response) => { const id = req.params.id; const { username } = req.user as { username: string }; + const { reason } = req.body; + + if (!reason || !reason.trim()) { + res.status(400).send({ + message: 'Rejection reason is required', + }); + return; + } // Get the push request const push = await getValidPushOrRespond(id, res); @@ -71,8 +79,29 @@ router.post('/:id/reject', async (req: Request, res: Response) => { const isAllowed = await db.canUserApproveRejectPush(id, username); if (isAllowed) { - const result = await db.reject(id, null); - console.log(`User ${username} rejected push request for ${id}`); + const reviewerList = await db.getUsers({ username }); + const reviewerEmail = reviewerList[0].email; + + if (!reviewerEmail) { + res.status(404).send({ + message: `There was no registered email address for the reviewer: ${username}`, + }); + return; + } + + const rejection = { + reason, + timestamp: new Date(), + reviewer: { + username, + reviewerEmail, + }, + }; + + const result = await db.reject(id, rejection); + console.log( + `User ${username} rejected push request for ${id}${reason ? ` with reason: ${reason}` : ''}`, + ); res.send(result); } else { res.status(403).send({ diff --git a/src/ui/services/git-push.ts b/src/ui/services/git-push.ts index eafe5c96d..05b526b98 100644 --- a/src/ui/services/git-push.ts +++ b/src/ui/services/git-push.ts @@ -99,12 +99,13 @@ const rejectPush = async ( id: string, setMessage: (message: string) => void, setUserAllowedToReject: (userAllowedToReject: boolean) => void, + reason?: string, ): Promise => { const apiV1Base = await getApiV1BaseUrl(); const url = `${apiV1Base}/push/${id}/reject`; let errorMsg = ''; let isUserAllowedToReject = true; - await axios.post(url, {}, getAxiosConfig()).catch((error: any) => { + await axios.post(url, { reason }, getAxiosConfig()).catch((error: any) => { if (error.response && error.response.status === 401) { errorMsg = 'You are not authorised to reject...'; isUserAllowedToReject = false; diff --git a/src/ui/views/PushDetails/PushDetails.tsx b/src/ui/views/PushDetails/PushDetails.tsx index aec01fa20..4892dc345 100644 --- a/src/ui/views/PushDetails/PushDetails.tsx +++ b/src/ui/views/PushDetails/PushDetails.tsx @@ -12,7 +12,9 @@ import CardFooter from '../../components/Card/CardFooter'; import Button from '../../components/CustomButtons/Button'; import Diff from './components/Diff'; import Attestation from './components/Attestation'; -import AttestationView from './components/AttestationView'; +import AttestationInfo from './components/AttestationInfo'; +import RejectionInfo from './components/RejectionInfo'; +import Reject from './components/Reject'; import Table from '@material-ui/core/Table'; import TableBody from '@material-ui/core/TableBody'; import TableHead from '@material-ui/core/TableHead'; @@ -21,11 +23,9 @@ import TableCell from '@material-ui/core/TableCell'; import { getPush, authorisePush, rejectPush, cancelPush } from '../../services/git-push'; import { CheckCircle, Visibility, Cancel, Block } from '@material-ui/icons'; import Snackbar from '@material-ui/core/Snackbar'; -import Tooltip from '@material-ui/core/Tooltip'; -import { AttestationFormData, PushActionView } from '../../types'; +import { PushActionView } from '../../types'; import { trimPrefixRefsHeads, trimTrailingDotGit } from '../../../db/helper'; import { generateEmailLink, getGitProvider } from '../../utils'; -import UserLink from '../../components/UserLink/UserLink'; const Dashboard: React.FC = () => { const { id } = useParams<{ id: string }>(); @@ -62,9 +62,9 @@ const Dashboard: React.FC = () => { } }; - const reject = async () => { + const reject = async (reason: string) => { if (!id) return; - await rejectPush(id, setMessage, setUserAllowedToReject); + await rejectPush(id, setMessage, setUserAllowedToReject, reason); if (isUserAllowedToReject) { navigate('/dashboard/push/'); } @@ -153,91 +153,17 @@ const Dashboard: React.FC = () => { - + )} - {push.attestation && push.authorised && ( -
- - { - if (!push.autoApproved) { - setAttestation(true); - } - }} - htmlColor='green' - /> - - - {push.autoApproved ? ( -
-

- Auto-approved by system -

-
- ) : ( - <> - {isGitHub && ( - - - - )} -
-

- {isGitHub && ( - - {push.attestation.reviewer.gitAccount} - - )} - {!isGitHub && }{' '} - approved this contribution -

-
- - )} - - - - {moment(push.attestation.timestamp).fromNow()} - - - - {!push.autoApproved && ( - - )} -
- )} + + diff --git a/src/ui/views/PushDetails/components/AttestationInfo.tsx b/src/ui/views/PushDetails/components/AttestationInfo.tsx new file mode 100644 index 000000000..5314be72f --- /dev/null +++ b/src/ui/views/PushDetails/components/AttestationInfo.tsx @@ -0,0 +1,105 @@ +import React from 'react'; +import moment from 'moment'; +import { CheckCircle } from '@material-ui/icons'; +import Tooltip from '@material-ui/core/Tooltip'; +import UserLink from '../../../components/UserLink/UserLink'; +import AttestationView from './AttestationView'; +import { AttestationFormData, PushActionView } from '../../../types'; + +interface AttestationInfoProps { + push: PushActionView; + isGitHub: boolean; + attestation: boolean; + setAttestation: (value: boolean) => void; +} + +const AttestationInfo: React.FC = ({ + push, + isGitHub, + attestation, + setAttestation, +}) => { + if (!push.attestation || !push.authorised) { + return null; + } + + return ( +
+ + { + if (!push.autoApproved) { + setAttestation(true); + } + }} + htmlColor='green' + /> + + + {push.autoApproved ? ( +
+

+ Auto-approved by system +

+
+ ) : ( + <> + {isGitHub && ( + + + + )} +
+

+ {isGitHub && ( + + {push.attestation.reviewer.gitAccount} + + )} + {!isGitHub && } approved + this contribution +

+
+ + )} + + + + {moment(push.attestation.timestamp).fromNow()} + + + + {!push.autoApproved && ( + + )} +
+ ); +}; + +export default AttestationInfo; diff --git a/src/ui/views/PushDetails/components/Reject.tsx b/src/ui/views/PushDetails/components/Reject.tsx new file mode 100644 index 000000000..5025b4443 --- /dev/null +++ b/src/ui/views/PushDetails/components/Reject.tsx @@ -0,0 +1,100 @@ +import React, { useState } from 'react'; +import Dialog from '@material-ui/core/Dialog'; +import DialogContent from '@material-ui/core/DialogContent'; +import DialogActions from '@material-ui/core/DialogActions'; +import TextField from '@material-ui/core/TextField'; +import { Block, ErrorOutline } from '@material-ui/icons'; +import Button from '../../../components/CustomButtons/Button'; + +interface RejectProps { + rejectFn: (reason: string) => void; +} + +const Reject: React.FC = ({ rejectFn }) => { + const [open, setOpen] = useState(false); + const [reason, setReason] = useState(''); + + const handleClickOpen = () => { + setOpen(true); + }; + + const handleClose = () => { + setOpen(false); + setReason(''); + }; + + const handleReject = () => { + if (!reason.trim()) { + return; + } + rejectFn(reason); + handleClose(); + }; + + return ( +
+ + + +
+ + + You are about to reject this contribution + +
+

+ This action will prevent this contribution from being published. +
+ Please provide a reason for rejection to help the contributor understand the decision. +

+ + + setReason(e.target.value)} + placeholder='Provide details about why this contribution is being rejected...' + required + /> + + + + + + +
+ ); +}; + +export default Reject; diff --git a/src/ui/views/PushDetails/components/RejectionInfo.tsx b/src/ui/views/PushDetails/components/RejectionInfo.tsx new file mode 100644 index 000000000..6af798c9d --- /dev/null +++ b/src/ui/views/PushDetails/components/RejectionInfo.tsx @@ -0,0 +1,107 @@ +import React from 'react'; +import moment from 'moment'; +import { Block } from '@material-ui/icons'; +import Tooltip from '@material-ui/core/Tooltip'; +import UserLink from '../../../components/UserLink/UserLink'; +import { PushActionView } from '../../../types'; + +interface RejectionInfoProps { + push: PushActionView; +} + +const RejectionInfo: React.FC = ({ push }) => { + if (!push.rejection || !push.rejected) { + return null; + } + + return ( +
+ + + + + {push.autoRejected ? ( +
+

+ Auto-rejected by system +

+
+ ) : ( + <> +
+

+ rejected this contribution +

+
+ + )} + + {push.rejection.reason && ( +
+

+ Reason +

+

+ {push.rejection.reason} +

+
+ )} + + + + {moment(push.rejection.timestamp).fromNow()} + + +
+ ); +}; + +export default RejectionInfo; diff --git a/test/db/file/pushes.test.ts b/test/db/file/pushes.test.ts new file mode 100644 index 000000000..5c23a36c2 --- /dev/null +++ b/test/db/file/pushes.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import * as pushesModule from '../../../src/db/file/pushes'; + +describe('File DB - Pushes', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('reject', () => { + it('should reject a push with rejection metadata', async () => { + const pushId = 'test-push-123'; + const mockPush = { + id: pushId, + authorised: false, + canceled: false, + rejected: false, + }; + + const rejection = { + reason: 'Code does not meet quality standards', + timestamp: new Date(), + reviewer: { + username: 'reviewer1', + reviewerEmail: 'reviewer1@example.com', + }, + }; + + // Mock db.findOne to return the push + vi.spyOn(pushesModule.db, 'findOne').mockImplementation((query: any, cb: any) => { + cb(null, mockPush); + }); + + // Mock db.update to succeed + vi.spyOn(pushesModule.db, 'update').mockImplementation( + (query: any, update: any, options: any, cb: any) => { + cb(null, 1); + }, + ); + + const result = await pushesModule.reject(pushId, rejection); + + expect(result).toEqual({ message: `reject ${pushId}` }); + expect(pushesModule.db.findOne).toHaveBeenCalled(); + expect(pushesModule.db.update).toHaveBeenCalledWith( + { id: pushId }, + expect.objectContaining({ + id: pushId, + authorised: false, + canceled: false, + rejected: true, + rejection: rejection, + }), + expect.any(Object), + expect.any(Function), + ); + }); + + it('should throw an error if push is not found', async () => { + const pushId = 'non-existent-push'; + + // Mock db.findOne to return null + vi.spyOn(pushesModule.db, 'findOne').mockImplementation((query: any, cb: any) => { + cb(null, null); + }); + + const rejection = { + reason: 'Test reason', + timestamp: new Date(), + reviewer: { + username: 'reviewer1', + reviewerEmail: 'reviewer1@example.com', + }, + }; + + await expect(pushesModule.reject(pushId, rejection)).rejects.toThrow( + `push ${pushId} not found`, + ); + expect(pushesModule.db.findOne).toHaveBeenCalledWith({ id: pushId }, expect.any(Function)); + }); + }); +}); diff --git a/test/db/mongo/pushes.test.ts b/test/db/mongo/pushes.test.ts new file mode 100644 index 000000000..7cce7446b --- /dev/null +++ b/test/db/mongo/pushes.test.ts @@ -0,0 +1,87 @@ +import { describe, it, expect, afterEach, vi, beforeEach } from 'vitest'; + +const mockFindOneDocument = vi.fn(); +const mockUpdateOne = vi.fn(); +const mockConnect = vi.fn(() => ({ + updateOne: mockUpdateOne, +})); + +vi.mock('../../../src/db/mongo/helper', () => ({ + connect: mockConnect, + findOneDocument: mockFindOneDocument, +})); + +describe('MongoDB - Pushes', async () => { + const { reject, authorise, getPush } = await import('../../../src/db/mongo/pushes'); + + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + describe('reject', () => { + it('should reject a push with rejection metadata', async () => { + const pushId = 'test-push-123'; + const mockPush = { + id: pushId, + authorised: false, + canceled: false, + rejected: false, + }; + + const rejection = { + reason: 'Code does not meet quality standards', + timestamp: new Date(), + reviewer: { + username: 'reviewer1', + reviewerEmail: 'reviewer1@example.com', + }, + }; + + mockFindOneDocument.mockResolvedValue(mockPush); + mockUpdateOne.mockResolvedValue({ modifiedCount: 1 }); + + const result = await reject(pushId, rejection); + + expect(result).toEqual({ message: `reject ${pushId}` }); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: pushId }); + expect(mockConnect).toHaveBeenCalledWith('pushes'); + + const [query, update, options] = mockUpdateOne.mock.calls[0]; + + expect(query).toEqual({ id: pushId }); + expect(options).toEqual({ upsert: true }); + expect(update.$set).toMatchObject({ + id: pushId, + authorised: false, + canceled: false, + rejected: true, + rejection: { + reason: rejection.reason, + reviewer: rejection.reviewer, + }, + }); + }); + + it('should throw an error if push is not found', async () => { + const pushId = 'non-existent-push'; + + mockFindOneDocument.mockResolvedValue(null); + + const rejection = { + reason: 'Test reason', + timestamp: new Date(), + reviewer: { + username: 'reviewer1', + reviewerEmail: 'reviewer1@example.com', + }, + }; + + await expect(reject(pushId, rejection)).rejects.toThrow(`push ${pushId} not found`); + expect(mockFindOneDocument).toHaveBeenCalledWith('pushes', { id: pushId }); + }); + }); +}); diff --git a/test/testPush.test.ts b/test/testPush.test.ts index 731ed69e5..27ad8bff7 100644 --- a/test/testPush.test.ts +++ b/test/testPush.test.ts @@ -274,10 +274,33 @@ describe('Push API', () => { await loginAsApprover(); const res = await request(app) .post(`/api/v1/push/${TEST_PUSH.id}/reject`) - .set('Cookie', `${cookie}`); + .set('Cookie', `${cookie}`) + .send({ reason: 'This contribution does not meet our standards' }); expect(res.status).toBe(200); }); + it('should NOT allow an authorizer to reject a push without a reason', async () => { + await db.writeAudit(TEST_PUSH as any); + await loginAsApprover(); + const res = await request(app) + .post(`/api/v1/push/${TEST_PUSH.id}/reject`) + .set('Cookie', `${cookie}`) + .send({}); + expect(res.status).toBe(400); + expect(res.body.message).toBe('Rejection reason is required'); + }); + + it('should NOT allow an authorizer to reject a push with empty reason', async () => { + await db.writeAudit(TEST_PUSH as any); + await loginAsApprover(); + const res = await request(app) + .post(`/api/v1/push/${TEST_PUSH.id}/reject`) + .set('Cookie', `${cookie}`) + .send({ reason: ' ' }); + expect(res.status).toBe(400); + expect(res.body.message).toBe('Rejection reason is required'); + }); + it('should NOT allow an authorizer to reject their own push', async () => { // make the approver also the committer const testPush = { ...TEST_PUSH }; @@ -287,7 +310,8 @@ describe('Push API', () => { await loginAsApprover(); const res = await request(app) .post(`/api/v1/push/${TEST_PUSH.id}/reject`) - .set('Cookie', `${cookie}`); + .set('Cookie', `${cookie}`) + .send({ reason: 'Testing rejection' }); expect(res.status).toBe(403); expect(res.body.message).toBe('Cannot reject your own changes'); }); @@ -301,7 +325,8 @@ describe('Push API', () => { await loginAsCommitter(); const res = await request(app) .post(`/api/v1/push/${pushWithOtherUser.id}/reject`) - .set('Cookie', `${cookie}`); + .set('Cookie', `${cookie}`) + .send({ reason: 'Testing rejection' }); expect(res.status).toBe(403); expect(res.body.message).toBe( 'User push-test-2 is not authorised to reject changes on this project',