-
Notifications
You must be signed in to change notification settings - Fork 158
Users can subscribe to the calendar #647
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
base: main
Are you sure you want to change the base?
Users can subscribe to the calendar #647
Conversation
f69bd35 to
ca1f85e
Compare
server/routes/calendar.js
Outdated
|
|
||
| module.exports = (app) => { | ||
| app.get("/calendar.ics", async (req, res) => { | ||
| try { |
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.
Not exactly sure how testing goes in this project, but I think it'd be good to write a test to make sure the ICS file is generated appropriately
Glad to help out on approach if you'd like!
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.
Thanks for the suggestion, and I agree that writing a test for this is important.
I'll add a server-side test that will make a GET request to the /calendar.ics endpoint and verify that the response status is 200, as well as that the Content-Type header is set to "text/calendar".
Would this be a good starting ground?
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.
Yep! Once you get that working, you can also check things like the filename and that the appropriate events are included in the ICS file!
It seems ics is for generating ics files and not parsing them, so you may need to include a dev dependency to validate or consider a library that does both
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.
I've attempted to write tests for my calendar.test.js. I ran npm test and all tests passed. However, I've had to make some changes to some other files under the test directory, along with my calendar.js route file. I am a bit wary of this result since there are errors generating in the console even though the tests all pass. Could I have a moment of your time to look through my new changes before I even consider committing?
bd645bf to
be2e602
Compare
server/routes/calendar.js
Outdated
| const ics = require("ics"); | ||
| const Event = require("../models/Event"); | ||
|
|
||
| const createDateArray = (date) => { |
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.
Since we have date-fns available we can use it, similar to what they used in the docs with moment (source).
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.
I have made changes to import the format function from date-fns and will be included in my next commit.
server/routes/calendar.js
Outdated
| // use createDateArray helper function | ||
| start: createDateArray(startDate), | ||
| end: createDateArray(endDate), | ||
| url: `https://together.rocks/events/${event._id}`, |
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.
This URL /events/:id does not work at the moment. Should we just send back the users to the /calendar page in the mean time?
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.
agree, hopefully we will get this behavior with #618
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.
I made changes to the url to send users to /calendar and will be included in my next commit.
server/routes/calendar.js
Outdated
| app.get("/calendar.ics", async (req, res) => { | ||
| try { | ||
| // fetch events from mongodb | ||
| const eventsFromDB = await Event.find().lean(); |
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.
From an user perspective, does it make sense to include all the events from the database?
We can proceed with this as the first iteration, but it's worth discussing for a future improvement.
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.
would this include past events as well? maybe we at least filter for events after the current date
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.
oh i did not take this into consideration! thanks for the suggestion. i'll put it on the back-burner for now and prioritize the .ics creation.
server/routes/calendar.js
Outdated
| // use createDateArray helper function | ||
| start: createDateArray(startDate), | ||
| end: createDateArray(endDate), | ||
| url: `https://together.rocks/events/${event._id}`, |
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.
Non blocker, but on the long run we need to think about together.rocks being hardcoded (tests, preview PR, change of domain, etc), same for the other <a> link in the client.
server/routes/calendar.js
Outdated
| const eventsFromDB = await Event.find().lean(); | ||
| // format events for the ics library | ||
| const icsEvents = eventsFromDB.map((event) => { | ||
| const startDate = event.startAt; |
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.
Why not destructure the whole object?
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.
i have refactored the map function to use object destructuring. i will include these changes in my next commit
|
super good overall and thank you for the contribution @himynameismoose, welcome in! |
be2e602 to
f3c0143
Compare
f3c0143 to
0196813
Compare
srpt3.mp4 |
Description
Issue #628: Added a button to the Calendar UI for users to subscribe to by adding the Together calendar to their personal calendar.
Dependencies added:
"ics": "^3.8.1"Type of change
Please select everything applicable. Please, do not delete any lines.
Issue
Checklist:
npm run testandnpm run test:e2eand all tests have passed successfully or I have included details within my PR on the failure.npm run lintand resolved any outstanding errors. Most issues can be solved by executingnpm run format