-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
Write events test suite #181
Changes from 10 commits
c12ac20
ef89fa3
bc43ff9
ce6dcb8
290c828
159d169
bbaedc2
6a9b5ae
74db6ba
d2fd439
8d7623d
a264273
caa4f92
a33bcd0
fcc0e4a
e56e157
cc6f83b
8ef3ea1
5b3fa0c
6e55d48
2f8c3b4
2529a86
28a55ca
ede38bf
539d35e
18a952c
342dcb5
a70a68e
cc1694b
9b51a89
3ecb774
2c5ca2a
a88706e
98023da
298f5f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ export default class EventService { | |
public async create(event: Event) { | ||
const eventCreated = await this.transactions.readWrite(async (txn) => { | ||
const eventRepository = Repositories.event(txn); | ||
const isUnusedAttendanceCode = eventRepository.isUnusedAttendanceCode(event.attendanceCode); | ||
const isUnusedAttendanceCode = await eventRepository.isUnusedAttendanceCode(event.attendanceCode); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you catch this with a test or just notice it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was caught with test - I saw that the duplicate attendance code error was thrown by Postgres' duplicate key constraint and not by the event service There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, our first bug caught by testing! |
||
if (!isUnusedAttendanceCode) throw new UserError('Attendance code has already been used'); | ||
return eventRepository.upsertEvent(EventModel.create(event)); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,21 +24,45 @@ export class EventFactory { | |
|
||
public static fake(): EventModel { | ||
const [start, end] = EventFactory.randomTime(); | ||
return EventFactory.fakeWithTime(start, end); | ||
} | ||
|
||
private static fakeWithTime(start: Date, end: Date): EventModel { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can use the private static fakeWithTime(start: Date, end: Date): EventModel {
return with({ start, end });
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Totally forgot about |
||
const event = EventFactory.fakeWithoutTime(); | ||
event.start = start; | ||
event.end = end; | ||
return event; | ||
} | ||
|
||
public static fakeWithoutTime(): EventModel { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the different between this and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As mentioned above, this method is not needed anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But the idea before was to have |
||
return EventModel.create({ | ||
uuid: uuid(), | ||
organization: FactoryUtils.pickRandomValue(EventFactory.ORGS), | ||
title: faker.random.hexaDecimal(10), | ||
description: faker.lorem.sentences(2), | ||
location: faker.random.hexaDecimal(10), | ||
start, | ||
end, | ||
attendanceCode: faker.random.hexaDecimal(10), | ||
pointValue: EventFactory.randomPointValue(), | ||
requiresStaff: FactoryUtils.getRandomBoolean(), | ||
staffPointBonus: EventFactory.randomPointValue(), | ||
}); | ||
} | ||
|
||
public static fakePastEvent(daysAgo = 1): EventModel { | ||
const [start, end] = EventFactory.randomPastTime(daysAgo); | ||
return EventFactory.fakeWithTime(start, end); | ||
} | ||
|
||
public static fakeOngoingEvent(): EventModel { | ||
const [start, end] = EventFactory.randomOngoingTime(); | ||
return EventFactory.fakeWithTime(start, end); | ||
} | ||
|
||
public static fakeFutureEvent(daysAhead = 1): EventModel { | ||
const [start, end] = EventFactory.randomFutureTime(daysAhead); | ||
return EventFactory.fakeWithTime(start, end); | ||
} | ||
|
||
private static randomTime(): [Date, Date] { | ||
// between last and next week | ||
const days = FactoryUtils.getRandomNumber(-7, 7); | ||
|
@@ -48,7 +72,41 @@ export class EventFactory { | |
const duration = FactoryUtils.getRandomNumber(30, 150, 30); | ||
const start = moment().subtract(days, 'days').hour(hour); | ||
const end = moment(start.valueOf()).add(duration, 'minutes'); | ||
return [new Date(start.valueOf()), new Date(end.valueOf())]; | ||
return [start.toDate(), end.toDate()]; | ||
} | ||
|
||
private static randomPastTime(daysAgo: number): [Date, Date] { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we reuse code between these |
||
// random day between daysAgo and a week before daysAgo | ||
const days = FactoryUtils.getRandomNumber(daysAgo, daysAgo + 7); | ||
// between 8 AM and 6 PM | ||
const hour = FactoryUtils.getRandomNumber(9, 19); | ||
// between 0.5 and 2.5 hours long, rounded to the half hour | ||
const duration = FactoryUtils.getRandomNumber(30, 150, 30); | ||
const start = moment().subtract(days, 'days').hour(hour); | ||
const end = moment(start.valueOf()).add(duration, 'minutes'); | ||
return [start.toDate(), end.toDate()]; | ||
} | ||
|
||
private static randomOngoingTime(): [Date, Date] { | ||
// 0-2 hours before now | ||
const hour = FactoryUtils.getRandomNumber(0, 2); | ||
// between 2.5 and 6 hours long, rounded to the half hour | ||
const duration = FactoryUtils.getRandomNumber(150, 480, 30); | ||
const start = moment().subtract(hour, 'hours'); | ||
const end = moment(start.valueOf()).add(duration, 'minutes'); | ||
return [start.toDate(), end.toDate()]; | ||
} | ||
|
||
private static randomFutureTime(daysAhead: number): [Date, Date] { | ||
// random day between daysAhead and a week before daysAhead | ||
const days = FactoryUtils.getRandomNumber(daysAhead, daysAhead + 7); | ||
// between 8 AM and 6 PM | ||
const hour = FactoryUtils.getRandomNumber(9, 19); | ||
// between 0.5 and 2.5 hours long, rounded to the half hour | ||
const duration = FactoryUtils.getRandomNumber(30, 150, 30); | ||
const start = moment().add(days, 'days').hour(hour); | ||
const end = moment(start.valueOf()).add(duration, 'minutes'); | ||
return [start.toDate(), end.toDate()]; | ||
} | ||
|
||
private static randomPointValue(): number { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,10 @@ export class FeedbackFactory { | |
}; | ||
} | ||
|
||
public static createEventFeedback(n: number): string[] { | ||
return new Array(n).fill(faker.random.word()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be a random word or some value from a predefined array of values? IIRC feedback's supposed to be some predefined emojis. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure the predefined emojis have been confirmed yet since a lot of the design materials are moving around right now, and membership hasn't acknowledged them either since the portal redesign has been pushed back significantly (till after the store frontend is finished). I think it's fine to keep them at random words for now and then adjust later once the set of emojis /text is confirmed. |
||
} | ||
|
||
private static randomFeedbackType(): FeedbackType { | ||
return FactoryUtils.pickRandomValue(Object.values(FeedbackType)); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
import { File } from '../../types'; | ||
|
||
const ONE_BYTE_FILLER = 'a'; | ||
|
||
// Factory for mocking in-memory files for testing with multer | ||
export class FileFactory { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we do test that we're writing/reading from S3 should we also verify the file is exactly the same? If so, maybe we write a random string and verify checksums? I don't think we need to actually interact with S3 though, mocking should be good enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, mocking should be good enough. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need this or can we mock the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'd still need this for verifying that a file that's too large gets rejected by the controller (the TODO test). |
||
public static image(size: number): File { | ||
const imageContents = ONE_BYTE_FILLER.repeat(size); | ||
const buffer = Buffer.from(imageContents); | ||
return { | ||
fieldname: 'image', | ||
originalname: 'image.jpg', | ||
filename: 'image.jpg', | ||
mimetype: 'image/jpg', | ||
buffer, | ||
encoding: '7bit', | ||
size, | ||
// the below fields are unused for in-memory multer storage but are still required by type def | ||
stream: null, | ||
destination: null, | ||
path: null, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,225 @@ | ||
import { validate } from 'class-validator'; | ||
import { plainToClass } from 'class-transformer'; | ||
import { DatabaseConnection, EventFactory, FeedbackFactory, FileFactory, PortalState, UserFactory } from './data'; | ||
import { ControllerFactory } from './controllers'; | ||
import { UserAccessType } from '../types'; | ||
import { Config } from '../config'; | ||
import { StorageUtils } from './utils'; | ||
import { SubmitEventFeedbackRequest } from '../api/validators/EventControllerRequests'; | ||
|
||
beforeAll(async () => { | ||
await DatabaseConnection.connect(); | ||
}); | ||
|
||
beforeEach(async () => { | ||
await DatabaseConnection.clear(); | ||
}); | ||
|
||
afterAll(async () => { | ||
await DatabaseConnection.clear(); | ||
await DatabaseConnection.close(); | ||
}); | ||
|
||
describe('event CRUD operations', () => { | ||
test('events from the past, future, and all time can be pulled', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const pastEvent = EventFactory.fakePastEvent(); | ||
const ongoingEvent = EventFactory.fakeOngoingEvent(); | ||
const futureEvent = EventFactory.fakeFutureEvent(); | ||
const user = UserFactory.fake(); | ||
|
||
await new PortalState() | ||
.createEvents([pastEvent, ongoingEvent, futureEvent]) | ||
.createUsers([user]) | ||
.write(); | ||
|
||
const eventController = ControllerFactory.event(conn); | ||
|
||
const pastEventsResponse = await eventController.getPastEvents({}, user); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: call the controller then assert instead of grouping all the responses then all the assertions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How come? I personally find it clearer to have all the assertions at the bottom so that it's easier to see what exactly is being asserted instead having assertions being tied within controller calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In general, it makes more sense to me to immediately assert instead of collecting all the responses so tests can fail faster and more obviously, e.g. if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Neither reason's particularly applicable here but I think it's still worth sticking to. |
||
const futureEventsResponse = await eventController.getFutureEvents({}, user); | ||
const allEventsResponse = await eventController.getAllEvents({}, user); | ||
|
||
expect(pastEventsResponse.events).toEqual( | ||
expect.arrayContaining([pastEvent.getPublicEvent()]), | ||
); | ||
expect(futureEventsResponse.events).toEqual( | ||
expect.arrayContaining([ | ||
ongoingEvent.getPublicEvent(), | ||
futureEvent.getPublicEvent(), | ||
]), | ||
); | ||
expect(allEventsResponse.events).toEqual( | ||
expect.arrayContaining([ | ||
pastEvent.getPublicEvent(), | ||
ongoingEvent.getPublicEvent(), | ||
futureEvent.getPublicEvent(), | ||
]), | ||
); | ||
}); | ||
|
||
test('events can be created with unused attendance codes', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const [event1] = EventFactory.with({ attendanceCode: 'code' }); | ||
const [event2] = EventFactory.with({ attendanceCode: 'different-code' }); | ||
const [admin] = UserFactory.with({ accessType: UserAccessType.ADMIN }); | ||
|
||
await new PortalState() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should prefer controllers wherever we can instead of relying on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I personally think it's clearer to use The one issue I'm seeing with |
||
.createEvents([event1]) | ||
.createUsers([admin]) | ||
.write(); | ||
|
||
const createEventResponse = await ControllerFactory.event(conn).createEvent({ event: event2 }, admin); | ||
|
||
expect(createEventResponse.error).toBeNull(); | ||
}); | ||
|
||
test('events cannot be created with duplicate attendance codes', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const [event1] = EventFactory.with({ attendanceCode: 'code' }); | ||
const [event2] = EventFactory.with({ attendanceCode: 'code' }); | ||
const [admin] = UserFactory.with({ accessType: UserAccessType.ADMIN }); | ||
|
||
await new PortalState() | ||
.createEvents([event1]) | ||
.createUsers([admin]) | ||
.write(); | ||
|
||
const eventController = ControllerFactory.event(conn); | ||
const errorMessage = 'Attendance code has already been used'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: inline this and the other error messages. |
||
|
||
await expect( | ||
eventController.createEvent({ event: event2 }, admin), | ||
).rejects.toThrow(errorMessage); | ||
|
||
const eventsResponse = await eventController.getAllEvents({}, admin); | ||
|
||
expect(eventsResponse.events).toHaveLength(1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Assert that the response is only the first event. |
||
}); | ||
}); | ||
|
||
describe('event covers', () => { | ||
const storage = new StorageUtils(); | ||
|
||
afterEach(async () => { | ||
await storage.deleteAllFilesInFolder(Config.file.EVENT_COVER_UPLOAD_PATH); | ||
}); | ||
|
||
test('properly updates cover photo in database and on S3', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const [event] = EventFactory.create(1); | ||
const [admin] = UserFactory.with({ accessType: UserAccessType.ADMIN }); | ||
const cover = FileFactory.image(Config.file.MAX_EVENT_COVER_FILE_SIZE / 2); | ||
|
||
await new PortalState() | ||
.createUsers([admin]) | ||
.createEvents([event]) | ||
.write(); | ||
|
||
const eventCoverResponse = await ControllerFactory.event(conn).updateEventCover(cover, event.uuid, admin); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think our tests should write to our prod bucket. At least, definitely not to the same folder. This is what I said a while back, that we should pass in our configs through the constructor, so we can specify where things get written to instead of being tied to whatever's in the prod config. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not too sure what we should pass in to the |
||
expect(eventCoverResponse.event.cover).toBeTruthy(); | ||
|
||
const eventCoverFiles = await storage.getAllFilesFromFolder(Config.file.EVENT_COVER_UPLOAD_PATH); | ||
expect(eventCoverFiles).toHaveLength(1); | ||
|
||
const fileName = StorageUtils.parseFirstFileNameFromFiles(eventCoverFiles); | ||
expect(fileName).toEqual(event.uuid); | ||
}); | ||
|
||
test('rejects upload if file size too large', async () => { | ||
// TODO: implement once API wrappers exist (since multer validation can't be mocked with function calls) | ||
}); | ||
}); | ||
|
||
describe('event feedback', () => { | ||
test('can be persisted and rewarded points when submitted for an event already attended', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const event = EventFactory.fakeOngoingEvent(); | ||
const user = UserFactory.fake(); | ||
const feedback = FeedbackFactory.createEventFeedback(3); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think |
||
|
||
await new PortalState() | ||
.createUsers([user]) | ||
.createEvents([event]) | ||
.attendEvents([user], [event], false) | ||
.write(); | ||
|
||
const previousPoints = user.points; | ||
|
||
await ControllerFactory.event(conn).submitEventFeedback(event.uuid, { feedback }, user); | ||
|
||
const attendanceResponse = await ControllerFactory.attendance(conn).getAttendancesForCurrentUser(user); | ||
const attendance = attendanceResponse.attendances[0]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: inline. |
||
|
||
expect(attendance.feedback).toEqual(feedback); | ||
|
||
expect(user.points).toEqual(previousPoints + Config.pointReward.EVENT_FEEDBACK_POINT_REWARD); | ||
shravanhariharan2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
|
||
test('is rejected on submission to an event not attended', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const event = EventFactory.fakeOngoingEvent(); | ||
const user = UserFactory.fake(); | ||
const feedback = FeedbackFactory.createEventFeedback(3); | ||
|
||
await new PortalState() | ||
.createUsers([user]) | ||
.createEvents([event]) | ||
.write(); | ||
|
||
const errorMessage = 'You must attend this event before submiting feedback'; | ||
|
||
await expect( | ||
ControllerFactory.event(conn).submitEventFeedback(event.uuid, { feedback }, user), | ||
).rejects.toThrow(errorMessage); | ||
}); | ||
|
||
test('is rejected if submitted to an event multiple times', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const event = EventFactory.fakeOngoingEvent(); | ||
const user = UserFactory.fake(); | ||
const feedback = FeedbackFactory.createEventFeedback(3); | ||
|
||
await new PortalState() | ||
.createUsers([user]) | ||
.createEvents([event]) | ||
.attendEvents([user], [event], false) | ||
.write(); | ||
|
||
const eventController = ControllerFactory.event(conn); | ||
await eventController.submitEventFeedback(event.uuid, { feedback }, user); | ||
|
||
const errorMessage = 'You cannot submit feedback for this event more than once'; | ||
|
||
await expect( | ||
eventController.submitEventFeedback(event.uuid, { feedback }, user), | ||
).rejects.toThrow(errorMessage); | ||
}); | ||
|
||
test('is rejected if sent after 2 days of event completion', async () => { | ||
const conn = await DatabaseConnection.get(); | ||
const event = EventFactory.fakePastEvent(3); | ||
const user = UserFactory.fake(); | ||
const feedback = FeedbackFactory.createEventFeedback(3); | ||
|
||
await new PortalState() | ||
.createUsers([user]) | ||
.createEvents([event]) | ||
.attendEvents([user], [event], false) | ||
.write(); | ||
|
||
const errorMessage = 'You must submit feedback within 2 days of the event ending'; | ||
|
||
await expect( | ||
ControllerFactory.event(conn).submitEventFeedback(event.uuid, { feedback }, user), | ||
).rejects.toThrow(errorMessage); | ||
}); | ||
|
||
test('is rejected if more than 3 feedback is provided', async () => { | ||
const feedback = FeedbackFactory.createEventFeedback(4); | ||
const errors = await validate(plainToClass(SubmitEventFeedbackRequest, { feedback })); | ||
|
||
expect(errors).toBeDefined(); | ||
expect(errors).toHaveLength(1); | ||
expect(errors[0].property).toEqual('feedback'); | ||
Comment on lines
+245
to
+247
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add an |
||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this change for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved SubmitEventFeedbackRequest from FeedbackControllerRequests to EventControllerRequests