Skip to content

Commit 4ae8abd

Browse files
andypolsandypols
committed
chore: enforce reject in api
1 parent c734811 commit 4ae8abd

File tree

3 files changed

+365
-5
lines changed

3 files changed

+365
-5
lines changed

‎src/service/routes/push.ts‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import express, { Request, Response } from 'express';
22
import * as db from '../../db';
33
import { PushQuery } from '../../db/types';
4+
import { convertIgnorePatternToMinimatch } from '@eslint/compat';
45

56
const router = express.Router();
67

@@ -43,6 +44,14 @@ router.post('/:id/reject', async (req: Request, res: Response) => {
4344
return;
4445
}
4546

47+
const reason = req.body.params?.reason;
48+
if (!reason || reason.trim().length === 0) {
49+
res.status(400).send({
50+
message: 'Rejection reason is required',
51+
});
52+
return;
53+
}
54+
4655
const id = req.params.id;
4756
const { username } = req.user as { username: string };
4857

@@ -72,8 +81,27 @@ router.post('/:id/reject', async (req: Request, res: Response) => {
7281
console.log({ isAllowed });
7382

7483
if (isAllowed) {
75-
const result = await db.reject(id, null);
76-
console.log(`user ${username} rejected push request for ${id}`);
84+
console.log(`user ${username} rejected push request for ${id} with reason: ${reason}`);
85+
86+
const reviewerList = await db.getUsers({ username });
87+
const reviewerEmail = reviewerList[0].email;
88+
89+
if (!reviewerEmail) {
90+
res.status(401).send({
91+
message: `There was no registered email address for the reviewer: ${username}`,
92+
});
93+
return;
94+
}
95+
96+
const rejection = {
97+
reason,
98+
timestamp: new Date(),
99+
reviewer: {
100+
username,
101+
reviewerEmail,
102+
},
103+
};
104+
const result = await db.reject(id, rejection);
77105
res.send(result);
78106
} else {
79107
res.status(401).send({

‎test/services/routes/push.test.js‎

Lines changed: 284 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,284 @@
1+
const chai = require('chai');
2+
const chaiHttp = require('chai-http');
3+
const sinon = require('sinon');
4+
const express = require('express');
5+
const proxyquire = require('proxyquire');
6+
7+
const { expect } = chai;
8+
chai.use(chaiHttp);
9+
10+
describe('Push API', () => {
11+
let app;
12+
let dbStub;
13+
let pushRouter;
14+
15+
const mockPush = {
16+
id: 'push-id-123',
17+
type: 'push',
18+
url: 'https://github.com/test/repo.git',
19+
userEmail: 'committer@example.com',
20+
user: 'testcommitter',
21+
cancelled: false,
22+
rejected: false,
23+
authorised: false,
24+
};
25+
26+
const createApp = (username) => {
27+
const app = express();
28+
app.use(express.json());
29+
app.use(express.urlencoded({ extended: true }));
30+
31+
if (username) {
32+
app.use((req, res, next) => {
33+
req.user = { username };
34+
next();
35+
});
36+
}
37+
38+
app.use('/push', pushRouter);
39+
return app;
40+
};
41+
42+
beforeEach(() => {
43+
dbStub = {
44+
getPush: sinon.stub(),
45+
getUsers: sinon.stub(),
46+
canCancelPush: sinon.stub(),
47+
cancel: sinon.stub(),
48+
canUserApproveRejectPush: sinon.stub(),
49+
reject: sinon.stub(),
50+
};
51+
52+
pushRouter = proxyquire('../../../src/service/routes/push', {
53+
'../../db': dbStub,
54+
}).default;
55+
});
56+
57+
afterEach(() => {
58+
sinon.restore();
59+
});
60+
61+
describe('POST /:id/reject', () => {
62+
it('should return 401 if user is not logged in', async () => {
63+
app = createApp(null);
64+
65+
const res = await chai
66+
.request(app)
67+
.post('/push/test-push-id-123/reject')
68+
.send({ params: { reason: 'test' } });
69+
70+
expect(res).to.have.status(401);
71+
expect(res.body).to.have.property('message', 'not logged in');
72+
});
73+
74+
it('should return 400 if rejection reason is missing', async () => {
75+
app = createApp('testuser');
76+
77+
const res = await chai.request(app).post('/push/test-push-id-123/reject').send({});
78+
79+
expect(res).to.have.status(400);
80+
expect(res.body).to.have.property('message', 'Rejection reason is required');
81+
});
82+
83+
it('should return 400 if rejection reason is empty string', async () => {
84+
app = createApp('testuser');
85+
86+
const res = await chai
87+
.request(app)
88+
.post('/push/test-push-id-123/reject')
89+
.send({ params: { reason: '' } });
90+
91+
expect(res).to.have.status(400);
92+
expect(res.body).to.have.property('message', 'Rejection reason is required');
93+
});
94+
95+
it('should return 400 if rejection reason is only whitespace', async () => {
96+
app = createApp('testuser');
97+
98+
const res = await chai
99+
.request(app)
100+
.post('/push/test-push-id-123/reject')
101+
.send({ params: { reason: ' ' } });
102+
103+
expect(res).to.have.status(400);
104+
expect(res.body).to.have.property('message', 'Rejection reason is required');
105+
});
106+
107+
it('should return 404 if push does not exist', async () => {
108+
app = createApp('testuser');
109+
dbStub.getPush.resolves(null);
110+
111+
const res = await chai
112+
.request(app)
113+
.post('/push/test-push-id-123/reject')
114+
.send({ params: { reason: 'Test reason' } });
115+
116+
expect(res).to.have.status(404);
117+
expect(res.body).to.have.property('message', 'Push request not found');
118+
});
119+
120+
it('should return 400 if push has not userEmail', async () => {
121+
app = createApp('testuser');
122+
const pushWithoutEmail = { ...mockPush, userEmail: null };
123+
dbStub.getPush.resolves(pushWithoutEmail);
124+
125+
const res = await chai
126+
.request(app)
127+
.post('/push/test-push-id-123/reject')
128+
.send({ params: { reason: 'Test reason' } });
129+
130+
expect(res).to.have.status(400);
131+
expect(res.body).to.have.property('message', 'Push request has no user email');
132+
});
133+
134+
it('should return 400 if no registered registered user', async () => {
135+
app = createApp('testuser');
136+
dbStub.getPush.resolves(mockPush);
137+
dbStub.getUsers.onFirstCall().resolves([]);
138+
139+
const res = await chai
140+
.request(app)
141+
.post('/push/test-push-id-123/reject')
142+
.send({ params: { reason: 'Test reason' } });
143+
144+
expect(res).to.have.status(401);
145+
expect(res.body.message).to.include('no registered user with the committer');
146+
});
147+
148+
it('should return 401 if user tries to reject their own push', async () => {
149+
app = createApp('testcommitter');
150+
dbStub.getPush.resolves(mockPush);
151+
dbStub.getUsers.onFirstCall().resolves([
152+
{
153+
username: 'testcommitter',
154+
email: 'committer@example.com',
155+
admin: false,
156+
},
157+
]);
158+
159+
const res = await chai
160+
.request(app)
161+
.post('/push/test-push-id-123/reject')
162+
.send({ params: { reason: 'Test reason' } });
163+
164+
expect(res).to.have.status(401);
165+
expect(res.body).to.have.property('message', 'Cannot reject your own changes');
166+
});
167+
168+
it('should allow admin to reject their own push', async () => {
169+
app = createApp('adminuser');
170+
dbStub.getPush.resolves({ ...mockPush, userEmail: 'admin@example.com' });
171+
dbStub.getUsers.onFirstCall().resolves([
172+
{
173+
username: 'adminuser',
174+
email: 'admin@example.com',
175+
admin: true,
176+
},
177+
]);
178+
dbStub.getUsers.onSecondCall().resolves([
179+
{
180+
username: 'adminuser',
181+
email: 'admin@example.com',
182+
admin: true,
183+
},
184+
]);
185+
dbStub.canUserApproveRejectPush.resolves(true);
186+
dbStub.reject.resolves({ message: 'reject test-push-123' });
187+
188+
const res = await chai
189+
.request(app)
190+
.post('/push/test-push-id-123/reject')
191+
.send({ params: { reason: 'Admin rejection' } });
192+
193+
expect(res).to.have.status(200);
194+
expect(dbStub.reject.calledOnce).to.be.true;
195+
});
196+
197+
it('should return 401 if user is not authorised to reject', async () => {
198+
app = createApp('unauthorizeduser');
199+
dbStub.getPush.resolves(mockPush);
200+
dbStub.getUsers.onFirstCall().resolves([
201+
{
202+
username: 'testcommitter',
203+
email: 'committer@example.com',
204+
admin: false,
205+
},
206+
]);
207+
dbStub.canUserApproveRejectPush.resolves(false);
208+
209+
const res = await chai
210+
.request(app)
211+
.post('/push/test-push-id-123/reject')
212+
.send({ params: { reason: 'Test reason' } });
213+
214+
expect(res).to.have.status(401);
215+
expect(res.body).to.have.property('message', 'User is not authorised to reject changes');
216+
});
217+
218+
it('should return 401 if reviewer has no email address', async () => {
219+
app = createApp('reviewer');
220+
dbStub.getPush.resolves(mockPush);
221+
dbStub.getUsers.onFirstCall().resolves([
222+
{
223+
username: 'testcommitter',
224+
email: 'committer@example.com',
225+
admin: false,
226+
},
227+
]);
228+
dbStub.getUsers.onSecondCall().resolves([
229+
{
230+
username: 'reviewer',
231+
admin: false,
232+
},
233+
]);
234+
dbStub.canUserApproveRejectPush.resolves(true);
235+
236+
const res = await chai
237+
.request(app)
238+
.post('/push/test-push-id-123/reject')
239+
.send({ params: { reason: 'Test reason' } });
240+
241+
expect(res).to.have.status(401);
242+
expect(res.body.message).to.include('no registered email address for the reviewer');
243+
});
244+
245+
it('should successfully reject a push with a valid reason', async () => {
246+
app = createApp('reviewer');
247+
dbStub.getPush.resolves(mockPush);
248+
dbStub.getUsers.onFirstCall().resolves([
249+
{
250+
username: 'testcommitter',
251+
email: 'committer@example.com',
252+
admin: false,
253+
},
254+
]);
255+
dbStub.canUserApproveRejectPush.resolves(true);
256+
dbStub.getUsers.onSecondCall().resolves([
257+
{
258+
username: 'reviewer',
259+
email: 'reviewer@example.com',
260+
},
261+
]);
262+
dbStub.reject.resolves({ message: 'reject test-push-123' });
263+
264+
const res = await chai
265+
.request(app)
266+
.post('/push/test-push-id-123/reject')
267+
.send({ params: { reason: 'Test reason' } });
268+
269+
expect(res).to.have.status(200);
270+
expect(res.body).to.have.property('message');
271+
272+
// Verify that the reject method was called with correct parameters
273+
expect(dbStub.reject.calledOnce).to.be.true;
274+
const [pushId, rejection] = dbStub.reject.firstCall.args;
275+
expect(pushId).to.equal('test-push-id-123');
276+
expect(rejection).to.have.property('reason', 'Test reason');
277+
expect(rejection.reviewer).to.deep.equal({
278+
username: 'reviewer',
279+
reviewerEmail: 'reviewer@example.com',
280+
});
281+
expect(rejection).to.have.property('timestamp').that.is.a('date');
282+
});
283+
});
284+
});

0 commit comments

Comments
 (0)