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

Diary refactor #530

Merged
merged 4 commits into from Nov 30, 2018

Conversation

Projects
None yet
3 participants
@hoyes
Member

hoyes commented Nov 30, 2018

https://diary-refactor.dev.camdram.net/diary.json
https://diary-refactor.dev.camdram.net/diary.xml
https://diary-refactor.dev.camdram.net/diary.ics

https://diary-refactor.dev.camdram.net/societies/society-3/diary.json
https://diary-refactor.dev.camdram.net/societies/society-3/diary.xml
https://diary-refactor.dev.camdram.net/societies/society-3/diary.ics

https://diary-refactor.dev.camdram.net/venues/theatre-royal-drury-lane/diary.json
https://diary-refactor.dev.camdram.net/venues/theatre-royal-drury-lane/diary.xml
https://diary-refactor.dev.camdram.net/venues/theatre-royal-drury-lane/diary.ics

https://diary-refactor.dev.camdram.net/vacancies/auditions/diary.json
https://diary-refactor.dev.camdram.net/vacancies/auditions/diary.xml
https://diary-refactor.dev.camdram.net/vacancies/auditions/diary.ics

@CHTJonas' diary RSS feed link wasn't really working so I've removed it. Is there a use case for a diary RSS feeds though?

Getting a bit pernickety, but I don't really like how start/end times are formatted at the moment:

"date": "2018-11-30T00:00:00+00:00",
"start_time": "1970-01-01T12:30:00+00:00",
"end_time": "1970-01-01T16:30:00+00:00",

Wondering about changing it everywhere to something like

"start_at": "2018-11-30T12:30:00+00:00",
"end_at": "2018-11-30T16:30:00+00:00",
"repeat_until": "2018-12-04T12:30:00",

, with "end_at" and "repeat_until" being optional. Means that for performances (which don't have an end time), "end_at" will be absent, and for auditions (which don't repeat) "repeat_until" will be absent. It also mirrors how iCal / RFC 5545 works.

@hoyes hoyes force-pushed the diary-refactor branch from 33352b5 to fa79769 Nov 30, 2018

@hoyes hoyes requested review from GKFX and CHTJonas Nov 30, 2018

@hoyes hoyes self-assigned this Nov 30, 2018

@hoyes hoyes force-pushed the diary-refactor branch from fa79769 to 7ffd48e Nov 30, 2018

@GKFX

GKFX approved these changes Nov 30, 2018

Great to see so much code deleted! All seems reasonable to me. Changing the way we store performance start/end times to the iCal approach would not be unreasonable, although in the interests of keeping this PR focused I'd add it to #386.

@CHTJonas

This comment has been minimized.

Member

CHTJonas commented Nov 30, 2018

Whoops my mistake didn't check to see if the RSS reader actually returned anything sensible! Yes all looks good to me.

To be honest I think iCal subscriptions are of much more use to people these days than RSS feeds. Only other thing I can think of is that it might be useful to implement some kind of plain text view of the diary for people to copy/paste into emails and alike but I think that may be going a bit OTT.

@CHTJonas CHTJonas merged commit 7ffd48e into master Nov 30, 2018

3 checks passed

buddy/pipeline/Development Deploy Build successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@CHTJonas CHTJonas deleted the diary-refactor branch Nov 30, 2018

@CHTJonas

This comment has been minimized.

Member

CHTJonas commented Dec 7, 2018

@hoyes

This comment has been minimized.

Member

hoyes commented Dec 7, 2018

Interesting. Never implemented, but seems obvious now you mention it.

Would ideally involve taking all the other endpoints in DiaryController and making it more generic somehow. Might involve turning everything into query params.

@CHTJonas

This comment has been minimized.

Member

CHTJonas commented Dec 7, 2018

Don't think it's a priority tbh. And yeah I think query patterns on /diary.json would be nicer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment