Skip to content

Commit 1bbe9e0

Browse files
mauberti-bcmauberti-bc
authored
SIMSBIOHUB-894: Bug fix to clear invalid cached carts (#349)
* clear invalid cached carts add record_end_date & tighten middleware linter add 404 test improve tests skip redundant get request on cart creation with features only clear on 401 403 404 fix pagination * promise all * clean up
1 parent 4fa6863 commit 1bbe9e0

File tree

15 files changed

+315
-70
lines changed

15 files changed

+315
-70
lines changed

‎api/src/models/cart.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@ export type CartSubmissionFeature = z.infer<typeof CartSubmissionFeature>;
2222
export const Cart = z.object({
2323
cart_id: z.string(),
2424
system_user_id: z.number().nullable(),
25-
cart_status: z.nativeEnum(CartStatus)
25+
cart_status: z.nativeEnum(CartStatus),
26+
record_end_date: z.string().nullable()
2627
});
2728

2829
export type Cart = z.infer<typeof Cart>;

‎api/src/openapi/schemas/cart.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,15 +59,16 @@ export const GetCartSubmissionFeaturesSchema: OpenAPIV3.SchemaObject = {
5959

6060
export const GetCartSchema: OpenAPIV3.SchemaObject = {
6161
type: 'object',
62-
required: ['cart_id', 'system_user_id', 'cart_status'],
62+
required: ['cart_id', 'system_user_id', 'cart_status', 'record_end_date'],
6363
additionalProperties: false,
6464
properties: {
6565
cart_id: { type: 'string', format: 'uuid' },
6666
system_user_id: { type: 'integer', nullable: true },
6767
cart_status: {
6868
type: 'string',
6969
enum: ['active', 'checked_out', 'expired', 'abandoned']
70-
}
70+
},
71+
record_end_date: { type: 'string', nullable: true }
7172
}
7273
};
7374

‎api/src/paths/cart/index.test.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ describe('cart', () => {
5151
cart: {
5252
cart_id: '1111-2222-3333-4444',
5353
system_user_id: 1,
54-
cart_status: CartStatus.ACTIVE
54+
cart_status: CartStatus.ACTIVE,
55+
record_end_date: null
5556
},
5657
features: [],
5758
pagination: {
@@ -90,7 +91,8 @@ describe('cart', () => {
9091
cart: {
9192
cart_id: '1111-2222-3333-4444',
9293
system_user_id: null,
93-
cart_status: CartStatus.ACTIVE
94+
cart_status: CartStatus.ACTIVE,
95+
record_end_date: null
9496
},
9597
features: [],
9698
pagination: {

‎api/src/paths/cart/{cartId}/index.test.ts‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,8 @@ describe('cart/{cartId}', () => {
5454
const fakeCart = {
5555
cart_id: 'cart-123',
5656
system_user_id: null,
57-
cart_status: CartStatus.ACTIVE
57+
cart_status: CartStatus.ACTIVE,
58+
record_end_date: null
5859
};
5960

6061
const fakePaginatedResponse = {
@@ -112,7 +113,8 @@ describe('cart/{cartId}', () => {
112113
const fakeCart = {
113114
cart_id: 'cart-123',
114115
system_user_id: null,
115-
cart_status: CartStatus.ACTIVE
116+
cart_status: CartStatus.ACTIVE,
117+
record_end_date: null
116118
};
117119

118120
const fakePaginatedResponse = {
@@ -161,7 +163,8 @@ describe('cart/{cartId}', () => {
161163
const fakeCart = {
162164
cart_id: 'cart-123',
163165
system_user_id: null,
164-
cart_status: CartStatus.ACTIVE
166+
cart_status: CartStatus.ACTIVE,
167+
record_end_date: null
165168
};
166169

167170
const fakePaginatedResponse = {
@@ -177,7 +180,7 @@ describe('cart/{cartId}', () => {
177180
};
178181

179182
sinon.stub(CartService.prototype, 'getCartById').resolves(fakeCart);
180-
sinon
183+
const getPaginatedCartFeaturesResponseStub = sinon
181184
.stub(CartSubmissionFeatureService.prototype, 'getPaginatedCartFeaturesResponse')
182185
.resolves(fakePaginatedResponse);
183186

@@ -189,9 +192,12 @@ describe('cart/{cartId}', () => {
189192

190193
await requestHandler(mockReq, mockRes, mockNext);
191194

192-
// Check that default pagination is set in request
193-
expect(mockReq.query.limit).to.equal('25');
194-
expect(mockReq.query.page).to.equal('1');
195+
expect(getPaginatedCartFeaturesResponseStub).to.have.been.calledOnceWith('cart-123', {
196+
page: 1,
197+
limit: 25,
198+
sort: undefined,
199+
order: undefined
200+
});
195201
expect(mockDBConnection.commit).to.have.been.calledOnce;
196202
expect(mockDBConnection.release).to.have.been.calledOnce;
197203
expect(mockRes.statusValue).to.equal(200);
@@ -233,7 +239,8 @@ describe('cart/{cartId}', () => {
233239
const fakeCart = {
234240
cart_id: 'cart-123',
235241
system_user_id: null,
236-
cart_status: CartStatus.ACTIVE
242+
cart_status: CartStatus.ACTIVE,
243+
record_end_date: null
237244
};
238245

239246
sinon.stub(CartService.prototype, 'getCartById').resolves(fakeCart);
@@ -265,7 +272,7 @@ describe('cart/{cartId}', () => {
265272
});
266273
sinon.stub(db, 'getAPIUserDBConnection').returns(mockDBConnection);
267274
sinon.stub(CartService.prototype, 'getCartById').rejects(new Error('Cart not found'));
268-
const featureStub = sinon.stub(CartSubmissionFeatureService.prototype, 'getPaginatedCartFeaturesResponse');
275+
sinon.stub(CartSubmissionFeatureService.prototype, 'getPaginatedCartFeaturesResponse');
269276

270277
const requestHandler = getCartWithFeaturesById();
271278
const { mockReq, mockRes, mockNext } = getRequestHandlerMocks();
@@ -278,7 +285,6 @@ describe('cart/{cartId}', () => {
278285
expect.fail('Expected handler to throw');
279286
} catch (error) {
280287
expect((error as ApiError).message).to.equal('Cart not found');
281-
expect(featureStub).to.not.have.been.called;
282288
expect(mockDBConnection.rollback).to.have.been.calledOnce;
283289
expect(mockDBConnection.release).to.have.been.calledOnce;
284290
}

‎api/src/paths/cart/{cartId}/index.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -117,19 +117,16 @@ export function getCartWithFeaturesById(): RequestHandler {
117117
const cartService = new CartService(connection);
118118
const cartSubmissionFeatureService = new CartSubmissionFeatureService(connection);
119119

120-
// Return first 25 features from page 1 if pagination not specified
121-
req.query.limit = req.query.limit || '25';
122-
req.query.page = req.query.page || '1';
123-
124120
const pagination = makePaginationOptionsFromRequest(req);
125121

126-
const cart = await cartService.getCartById(cartId);
127-
128-
const paginatedFeatures = await cartSubmissionFeatureService.getPaginatedCartFeaturesResponse(cartId, pagination);
122+
const [cart, paginatedFeatures] = await Promise.all([
123+
cartService.getCartById(cartId),
124+
cartSubmissionFeatureService.getPaginatedCartFeaturesResponse(cartId, pagination)
125+
]);
129126

130127
await connection.commit();
131128

132-
res.status(200).json({
129+
return res.status(200).json({
133130
...paginatedFeatures,
134131
cart
135132
});

‎api/src/repositories/cart-repository.test.ts‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ describe('CartRepository', () => {
1919
const mockCart: Cart = {
2020
cart_id: 'cart-1',
2121
cart_status: CartStatus.ACTIVE,
22-
system_user_id: 1
22+
system_user_id: 1,
23+
record_end_date: null
2324
};
2425

2526
const mockQueryResponse = {
@@ -65,7 +66,8 @@ describe('CartRepository', () => {
6566
const mockCart: Cart = {
6667
cart_id: 'cart-1',
6768
cart_status: CartStatus.ABANDONED,
68-
system_user_id: 1
69+
system_user_id: 1,
70+
record_end_date: null
6971
};
7072

7173
const mockQueryResponse = {
@@ -107,7 +109,8 @@ describe('CartRepository', () => {
107109
const mockCart: Cart = {
108110
cart_id: 'cart-1',
109111
cart_status: CartStatus.ACTIVE,
110-
system_user_id: 1
112+
system_user_id: 1,
113+
record_end_date: null
111114
};
112115

113116
const mockQueryResponse = {

‎api/src/repositories/cart-repository.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,9 @@ export class CartRepository extends BaseRepository {
2020
*/
2121
async findCartById(cartId: string): Promise<Cart | null> {
2222
const knex = getKnex();
23-
const query = knex('cart').where('cart_id', cartId).select('cart_id', 'cart_status', 'system_user_id');
23+
const query = knex('cart')
24+
.where('cart_id', cartId)
25+
.select('cart_id', 'cart_status', 'system_user_id', 'record_end_date');
2426

2527
const response = await this.connection.knex(query, Cart);
2628

@@ -36,7 +38,9 @@ export class CartRepository extends BaseRepository {
3638
*/
3739
async getCartById(cartId: string): Promise<Cart> {
3840
const knex = getKnex();
39-
const query = knex('cart').where('cart_id', cartId).select('cart_id', 'cart_status', 'system_user_id');
41+
const query = knex('cart')
42+
.where('cart_id', cartId)
43+
.select('cart_id', 'cart_status', 'system_user_id', 'record_end_date');
4044

4145
const response = await this.connection.knex(query, Cart);
4246

@@ -68,7 +72,7 @@ export class CartRepository extends BaseRepository {
6872
system_user_id: systemUserId,
6973
cart_status: CartStatus.ACTIVE
7074
})
71-
.returning(['cart_id', 'cart_status', 'system_user_id']);
75+
.returning(['cart_id', 'cart_status', 'system_user_id', 'record_end_date']);
7276

7377
const response = await this.connection.knex(query, Cart);
7478

‎api/src/services/authorization/authorization-service.test.ts‎

Lines changed: 110 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -541,7 +541,8 @@ describe('authorizeByCart', function () {
541541
const fakeCart: Cart = {
542542
cart_id: 'cart-1',
543543
cart_status: CartStatus.ACTIVE,
544-
system_user_id: 1
544+
system_user_id: 1,
545+
record_end_date: null
545546
};
546547

547548
const systemUser: SystemUser = {
@@ -569,7 +570,8 @@ describe('authorizeByCart', function () {
569570
const fakeCart: Cart = {
570571
cart_id: 'cart-1',
571572
system_user_id: 1,
572-
cart_status: CartStatus.ACTIVE
573+
cart_status: CartStatus.ACTIVE,
574+
record_end_date: null
573575
};
574576

575577
sinon.stub(CartService.prototype, 'findCartById').resolves(fakeCart);
@@ -592,7 +594,8 @@ describe('authorizeByCart', function () {
592594
const fakeCartWithDifferentOwner: Cart = {
593595
cart_id: 'cart-1',
594596
cart_status: CartStatus.ACTIVE,
595-
system_user_id: 2
597+
system_user_id: 2,
598+
record_end_date: null
596599
};
597600
sinon.stub(CartService.prototype, 'findCartById').resolves(fakeCartWithDifferentOwner);
598601
sinon.stub(AuthorizationService.prototype, 'getCachedSystemUser').resolves(systemUser);
@@ -612,7 +615,8 @@ describe('authorizeByCart', function () {
612615
const fakeUnauthenticatedCart: Cart = {
613616
cart_id: 'cart-1',
614617
cart_status: CartStatus.ACTIVE,
615-
system_user_id: null // No system_user_id, indicating it's unauthenticated
618+
system_user_id: null, // No system_user_id, indicating it's unauthenticated
619+
record_end_date: null
616620
};
617621
sinon.stub(CartService.prototype, 'findCartById').resolves(fakeUnauthenticatedCart);
618622
sinon.stub(AuthorizationService.prototype, 'getCachedSystemUser').resolves(systemUser);
@@ -656,6 +660,108 @@ describe('authorizeByCart', function () {
656660

657661
expect(result).to.be.false;
658662
});
663+
664+
it('returns false if cart is checked out', async function () {
665+
const mockDBConnection = getMockDBConnection();
666+
sinon.stub(CartService.prototype, 'findCartById').resolves({
667+
cart_id: 'cart-1',
668+
cart_status: CartStatus.CHECKED_OUT,
669+
system_user_id: 1,
670+
record_end_date: null
671+
});
672+
sinon.stub(AuthorizationService.prototype, 'getCachedSystemUser').resolves(systemUser);
673+
674+
const authorizationService = new AuthorizationService(mockDBConnection);
675+
676+
const result = await authorizationService.authorizeByCart({
677+
discriminator: 'Cart',
678+
cartId: 'cart-1'
679+
});
680+
681+
expect(result).to.be.false;
682+
});
683+
684+
it('returns false if cart is expired', async function () {
685+
const mockDBConnection = getMockDBConnection();
686+
sinon.stub(CartService.prototype, 'findCartById').resolves({
687+
cart_id: 'cart-1',
688+
cart_status: CartStatus.EXPIRED,
689+
system_user_id: 1,
690+
record_end_date: null
691+
});
692+
sinon.stub(AuthorizationService.prototype, 'getCachedSystemUser').resolves(systemUser);
693+
694+
const authorizationService = new AuthorizationService(mockDBConnection);
695+
696+
const result = await authorizationService.authorizeByCart({
697+
discriminator: 'Cart',
698+
cartId: 'cart-1'
699+
});
700+
701+
expect(result).to.be.false;
702+
});
703+
704+
it('returns false if cart is abandoned', async function () {
705+
const mockDBConnection = getMockDBConnection();
706+
sinon.stub(CartService.prototype, 'findCartById').resolves({
707+
cart_id: 'cart-1',
708+
cart_status: CartStatus.ABANDONED,
709+
system_user_id: 1,
710+
record_end_date: null
711+
});
712+
sinon.stub(AuthorizationService.prototype, 'getCachedSystemUser').resolves(systemUser);
713+
714+
const authorizationService = new AuthorizationService(mockDBConnection);
715+
716+
const result = await authorizationService.authorizeByCart({
717+
discriminator: 'Cart',
718+
cartId: 'cart-1'
719+
});
720+
721+
expect(result).to.be.false;
722+
});
723+
724+
it('returns false if cart record_end_date is in the past', async function () {
725+
const mockDBConnection = getMockDBConnection();
726+
sinon.stub(CartService.prototype, 'findCartById').resolves({
727+
cart_id: 'cart-1',
728+
cart_status: CartStatus.ACTIVE,
729+
system_user_id: 1,
730+
record_end_date: '2000-01-01T00:00:00.000Z'
731+
});
732+
sinon.stub(AuthorizationService.prototype, 'getCachedSystemUser').resolves(systemUser);
733+
734+
const authorizationService = new AuthorizationService(mockDBConnection);
735+
736+
const result = await authorizationService.authorizeByCart({
737+
discriminator: 'Cart',
738+
cartId: 'cart-1'
739+
});
740+
741+
expect(result).to.be.false;
742+
});
743+
744+
it('returns true if cart record_end_date is in the future and user owns the cart', async function () {
745+
const mockDBConnection = getMockDBConnection();
746+
sinon.stub(CartService.prototype, 'findCartById').resolves({
747+
cart_id: 'cart-1',
748+
cart_status: CartStatus.ACTIVE,
749+
system_user_id: 1,
750+
record_end_date: '2999-01-01T00:00:00.000Z'
751+
});
752+
753+
const systemUser = { system_user_id: 1 } as SystemUserExtended;
754+
sinon.stub(AuthorizationService.prototype, 'getCachedSystemUser').resolves(systemUser);
755+
756+
const authorizationService = new AuthorizationService(mockDBConnection);
757+
758+
const result = await authorizationService.authorizeByCart({
759+
discriminator: 'Cart',
760+
cartId: 'cart-1'
761+
});
762+
763+
expect(result).to.be.true;
764+
});
659765
});
660766

661767
describe('hasAtLeastOneValidValue', () => {

‎api/src/services/authorization/authorization-service.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import dayjs from 'dayjs';
12
import { SYSTEM_ROLE } from '../../constants/roles';
23
import { IDBConnection } from '../../database/db';
34
import { SystemUser, SystemUserExtended } from '../../repositories/user-repository';
@@ -260,6 +261,18 @@ export class AuthorizationService extends DBService {
260261
return false;
261262
}
262263

264+
// Only active carts are accessible
265+
if (cart.cart_status !== 'active') {
266+
return false;
267+
}
268+
269+
// Deny access to carts whose validity window has ended
270+
const recordEndDate = cart.record_end_date ? dayjs(cart.record_end_date) : null;
271+
const cartValidityEnded = recordEndDate !== null && (!recordEndDate.isValid() || !recordEndDate.isAfter(dayjs()));
272+
if (cartValidityEnded) {
273+
return false;
274+
}
275+
263276
// Ensure we have the current system user
264277
const currentUser = await this.getCachedSystemUser();
265278

0 commit comments

Comments
 (0)