GreenStepsChatt / greensteps

This is the web app for Green Steps, a community focused initiative that incentivizes litter cleanup in the Chattanooga area.
https://www.greenstepschatt.com/
MIT License
12 stars 31 forks source link

Add confirmable to User #99

Closed invacuo closed 6 years ago

invacuo commented 6 years ago

Description

Fixes #88

Developer Checklist

You can mark items off by replacing [ ] with [x] or in the rendered markdown, you can simply check the checkbox.

Maintainer Checklist

Screenshots

Warning that a user will see after 1 day

screen shot 2017-11-25 at 12 29 36 am

After clicking Didn't receive confirmation instructions?

screen shot 2017-11-25 at 12 06 10 am

Confirmation email copy (via letter_opener)

screen shot 2017-11-25 at 12 29 57 am

After confirming email

screen shot 2017-11-25 at 12 30 10 am
crawfoal commented 6 years ago

Thanks for your help! I have a few questions:

invacuo commented 6 years ago
invacuo commented 6 years ago

Sorry, I forgot to pull your changes in so I accidentally removed the newline fix commit you had.

crawfoal commented 6 years ago

Things look good here, I'm just thinking about how I want to set up the different environments. I have Heroku pipelines set up, so in addition to the typical dev and test environments, there are review apps and a staging app. The review and staging apps are all running in the production Rails environment. They are also all automatically populated with sample data where the users have fake email addresses.

Right now, I'm thinking about creating a staging Rails environment and having the review apps and the current staging app run in this environment. Then I could create another staging app that runs the production Rails environment and doesn't have sample data. We could create users in there with our real emails and then when we want to test stuff with mailers, we'd use this new staging app.

Do you have any input on this?

invacuo commented 6 years ago
crawfoal commented 6 years ago

I think that accidentally sending emails to real users is only a concern if you copy the production database to a staging or preview app.

Right now, the staging app creates sample data with fake emails (e.g. "user1@example.com"). For the other environment I'm thinking of creating, we would create users and other data by hand. We'd use our own email addresses so that we could make sure everything is configured correctly, so we'd only ever be at risk of emailing people who have created test users.

crawfoal commented 6 years ago

I've thought about this more and decided not to create another Rails environment. I'm just going to do something like what you mentioned earlier - whitelist email addresses that can be mailed to.

Also, I created a spec for the deploy task and I was thinking it was silly to test something so simple, but then it didn't pass. Am I missing something here?

require 'rails_helper'

RSpec.describe 'rake after_party:confirm_existing_users', type: :task do
  it 'marks existing users as confirmed' do
    user = create :user

    task.execute

    expect(user).to be_confirmed
  end
end
invacuo commented 6 years ago

Is there a stack trace or an output that you can paste?

crawfoal commented 6 years ago

Sorry for the delayed response 😕

I looked back over this and realized that it was just because the user record needed to be reloaded before the expectation.

I've added this spec and modified the deploy task to use the Rails logger instead of puts (mostly so that we don't have output in the middle of the RSpec run). I pushed these changes to a branch on my remote (I also merged in master). If these changes look good to you, then either one of us can merge them into this PR and then I'm ready to merge this PR in (that sounds overly complex now that I'm typing, but I didn't want to merge those changes directly into your branch without asking first 🙂 ).

invacuo commented 6 years ago

Just left one comment about keeping the puts. Besides that it looks good! Feel free to merge the changes into my branch if that's easier.

Thanks! 😄