CoderDojoGitHub / our-dojo

Our dojo's website, used for archiving lesson plans and signing up students for classes
http://coderdojosf.com
5 stars 4 forks source link

Spike public lesson and event site #10

Closed jonmagic closed 11 years ago

jonmagic commented 11 years ago

Based on #9

Home page

homepage

Lessons

lessons

Lesson Without Upcoming Event

lesson

Lesson With Upcoming Event Registration Not Yet Open

lesson-with-upcoming-event-registration-not-yet-open

Lesson With Upcoming Event Registration Open

lesson-with-upcoming-event-registration-open

Registration Open Html Email

registration-open-html-email

Registration Open Text Email

registration-open-text-email

Registration Confirmed Html Email

registration-confirmed-html-email

Registration Confirmed Text Email

registration-confirmed-text-email

jonmagic commented 11 years ago

Ready for review.

There are a couple issues that still need to happen before this is functioning the way I want, but they can come in separate PR's.

Also, design is being worked on by @jcollyer on his fork and we'll probably be looking for feedback in the next week or two as we iterate on it.

/cc @cameronmcefee @KartikTalwar @brntbeer

jonmagic commented 11 years ago

Hmm, I don't have screenshots of the emails. I'll work on that.

brntbeer commented 11 years ago

https://github.com/CoderDojoSF/event-app/blob/spike-public-lesson-and-event-site/app/models/event_notifier.rb#L26-L39

Also, I'm sure you're ahead of this already, but is this where you're double checking that we don't hit people with a ton of emails right? I don't want to send multiple emails, ie: Registered at last event will check that people are not already registered or signed up to receive an email notification right?

Just want to make sure we're not sending duplicates!

jonmagic commented 11 years ago

https://github.com/CoderDojoSF/event-app/blob/spike-public-lesson-and-event-site/app/models/event_notifier.rb#L28 stops a notification going out for an event subscriber if it has already been sent. sent_at gets set here https://github.com/CoderDojoSF/event-app/blob/spike-public-lesson-and-event-site/app/models/event_notifier.rb#L33 and saved here https://github.com/CoderDojoSF/event-app/blob/spike-public-lesson-and-event-site/app/models/event_notifier.rb#L34

On Sunday, May 5, 2013 at 4:21 PM, Brent Beer wrote:

https://github.com/CoderDojoSF/event-app/blob/spike-public-lesson-and-event-site/app/models/event_notifier.rb#L26-L39 Also, I'm sure you're ahead of this already, but is this where you're double checking that we don't hit people with a ton of emails right? I don't want to send multiple emails, ie: Registered at last event will check that people are not already registered or signed up to receive an email notification right?
Just want to make sure we're not sending duplicates!

— Reply to this email directly or view it on GitHub (https://github.com/CoderDojoSF/event-app/pull/10#issuecomment-17461316).

brntbeer commented 11 years ago

Rad! That's what i was thinking

cameronmcefee commented 11 years ago

Nice! This is looking awesome. I'd like to make some voice tweaks to the email language at some point. Should I do that here or wait until later?

Since this app relies so heavily on email, I feel like we ought to have unsubscribe links in emails, or some sort of "this is a one time email" type blurb at the bottom of the email. Nothing to obnoxious, but enough to let people know we're not going to be blowing up their email.

jonmagic commented 11 years ago

I think we should wait for this PR to get merged. It is already massive and I'd like to get back to small PR's that fix specific things.

I solicited getting a code review but haven't had any bites yet so I may just go over everything myself once more and then land this PR so we can start making it awesome with subsequent ones.

On Monday, May 6, 2013 at 8:45 AM, Cameron McEfee wrote:

Nice! This is looking awesome. I'd like to make some voice tweaks to the email language at some point. Should I do that here or wait until later? Since this app relies so heavily on email, I feel like we ought to have unsubscribe links in emails, or some sort of "this is a one time email" type blurb at the bottom of the email. Nothing to obnoxious, but enough to let people know we're not going to be blowing up their email.

— Reply to this email directly or view it on GitHub (https://github.com/CoderDojoSF/event-app/pull/10#issuecomment-17489513).

brntbeer commented 11 years ago

Trying to code review as best as possible ;), still haven't run it locally. The email stuff is the part that i would be most unsure of how to run around with locally though

jonmagic commented 11 years ago

I need to do quite a bit more documentation in README for getting setup at this point, to explain how to get email working and stuff.

On Monday, May 6, 2013 at 9:28 AM, Brent Beer wrote:

Trying to code review as best as possible ;), still haven't run it locally. The email stuff is the part that i would be most unsure of how to run around with locally though

— Reply to this email directly or view it on GitHub (https://github.com/CoderDojoSF/event-app/pull/10#issuecomment-17492069).

brntbeer commented 11 years ago

Well, after you go through the app once more, i say ship it and then we can work on some smaller iteration type stuff (and fixes, honestly if they're needed)