feat: add community events

ojongerius commented 6 years ago

Not ready for QA but this adds visibility and facilitates discussion about the implementation.

I'll keep pagination (#90), dataloaders (#38) and query depth limitation (#89) out of scope of this PR, but those things are exactly what I aim to work after Events go in. All of those things make sense when we have events in place, and will help mature the project.

[x] Implement methods src/dataLayer/mongo/communityEvent.js
[x] Agree on using an external uuid that is not coupled to any implementation
[x] Write tests
[x] Update integration test snapshot -should return errors for a query skipping a mandatory field: skipped mandatory field error 1

ojongerius commented 6 years ago

Did some work on this today.

Getting of one or more events needs tidying up but works.

I've hacked up a create function, but am not feeling much love from MongoDb in regards to referencing the User collection in the Events collection.

ojongerius commented 6 years ago

I've finally gotten some love from MongoDb, but much tidying up to do before I push those changes. Should be able to get this reviewable for a first cut this week, hopefully tomorrow.

ojongerius commented 6 years ago

Finally started with writing tests 🙌

ojongerius commented 6 years ago

All CRUD methods apart from update have been implemented, all implemented methods apart from getEvents (plural) have tests in src/dataLayer/mongo/event-datalayer.test.js 🎉

▶ yarn test src/dataLayer/mongo/event-datalayer.test.js
yarn run v1.6.0
$ cross-env JWT_CERT=test jest --runInBand  --verbose --silent src/dataLayer/mongo/event-datalayer.test.js
 PASS  src/dataLayer/mongo/event-datalayer.test.js
  createCommunityEvent
    ✓ should return an Event object (13ms)
    ✓ should throw if an attendee does yet exist (4ms)
  getCommunityEvent
    ✓ should return an Event object for a valid request (5ms)
    ✓ title search should throw when no matching events are found (1ms)
    ✓ externalId search should throw when no matching events are found (1ms)
    ✓ should throw if the externalId is not valid (2ms)
    ✓ should throw if the title is not valid (1ms)
  deleteCommunityEvent
    ✓ should delete an existing event (12ms)
    ✓ should return with an error for a non existing event (3ms)
    ✓ should refuse deletion of events owned by other users (3ms)

Test Suites: 1 passed, 1 total
Tests:       10 passed, 10 total
Snapshots:   2 passed, 2 total
Time:        1.268s, estimated 2s
✨  Done in 3.56s.

My next steps will be to:

Add test for population and getEvents
Remove duplication, and more tidying of communityEvent.js

I'll refrain from implementing the update function for now. My main goals were to have something that could do with Pagination (getEvents being one), dataloader and query depth limitation. A nice upshot is that we have a model and framework for events, when that will be implemented.

My thoughts are to get this ready for review starting next week if time permits. I'd love to get it in by mid week so I can start working on Pagination, dataloader and query depth limitation.

ojongerius commented 6 years ago

Current status:

[x] Add tests for events and population
[x] Remove duplication and tidy up

ojongerius commented 6 years ago

Things that I'm aware of and could/should improve:

[x] Schema validation
[x] An integration test
[x] Error handling, currently a mix of reject and throwing, this could do with some TLC
[x] A lot of repetition in the unit test

ojongerius commented 6 years ago

At the off chance someone was going to review this; I've got an integration test written up, but not yet committed. This included refactoring error handling, two birds one stone 😄

Running both integration and unit test will cause a collision in user creation, I'll tidy that up before I push things out. That will be either this evening or tomorrow.

ojongerius commented 6 years ago

I'm raising issues for all boxes that I ticked but did not address yet:

Agree on using an external uuid that is not coupled to any implementation: #184
Update integration test snapshot -should return errors for a query skipping a mandatory field: skipped mandatory field error 1 #185
Schema validation #186
Create update funciton #187
A lot of repetition in unit tests #188

ojongerius commented 6 years ago

I'm sure this could do with a lot of improvements, and I'm happy to revisit some of the work. For now I'm going to merge this so I can start work on related issues without a future merge nightmare.

raisedadead commented 6 years ago

@ojongerius I just want to extend my thanks for taking this up! You are absolutely the rockstar man.

sivakar12 commented 6 years ago

@ojongerius There is something wrong with the test "should refuse deletion of events owned by other users". The assertions are not happening. Add expect.assertions(3) on top and you will see the test failing saying that it received zero assertion calls. I found this when I tried to do something similar for updateCommunityEvent test I think the reason for this is that User objects in the database don't have externalId and the owner field is not populated. So null == null and everything seems to be proceeding like normal

freeCodeCamp / open-api

feat: add community events #177