Skip to content
This repository has been archived by the owner on Oct 21, 2020. It is now read-only.

feat: add community events #177

Merged
merged 1 commit into from
Jun 7, 2018

Conversation

ojongerius
Copy link
Contributor

@ojongerius ojongerius commented May 25, 2018

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.

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

@ojongerius ojongerius changed the title feat: add community events feat: add community events (WIP) May 25, 2018
@ojongerius ojongerius force-pushed the feat/events branch 2 times, most recently from f18be94 to ec9f50e Compare May 28, 2018 02:22
@ojongerius
Copy link
Contributor Author

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
Copy link
Contributor Author

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 ojongerius force-pushed the feat/events branch 2 times, most recently from 57c8791 to e2cea7b Compare May 29, 2018 05:29
@ojongerius
Copy link
Contributor Author

Finally started with writing tests 🙌

@ojongerius
Copy link
Contributor Author

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
Copy link
Contributor Author

ojongerius commented Jun 5, 2018

Current status:

  • Add tests for events and population
  • Remove duplication and tidy up

@ojongerius ojongerius changed the title feat: add community events (WIP) feat: add community events Jun 6, 2018
@ojongerius
Copy link
Contributor Author

ojongerius commented Jun 6, 2018

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

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

@ojongerius
Copy link
Contributor Author

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 ojongerius changed the title feat: add community events feat: add community events (WIP) Jun 6, 2018
@ojongerius
Copy link
Contributor Author

ojongerius commented Jun 7, 2018

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

@ojongerius
Copy link
Contributor Author

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.

@ojongerius ojongerius merged commit 6fad285 into freeCodeCamp:staging Jun 7, 2018
@ojongerius ojongerius deleted the feat/events branch June 7, 2018 04:58
@raisedadead
Copy link
Member

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

@sivakar12
Copy link
Contributor

sivakar12 commented Jun 13, 2018

@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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants