search

VeteransOnWatch / veterans-on-watch-server

MIT License
0 stars 0 forks source link

2 devise scaffold #8

Closed lightmedium closed 10 years ago

lightmedium commented 10 years ago

You'll need to run "bundle install" and "rake db:migrate"

ztbrown commented 10 years ago

I pushed some suggestions. It was Rspec + Shoulda + some db tests for User. Check them out and accept or rollback?

I would also suggest that we delete app/javascripts, app/stylesheets, and app/views before merging.

ztbrown commented 10 years ago

Hey, I just had a moment of, "Oh man, it might not have been cool of me to make updates to this branch!!" That's how we do it over here and I think it must be force-of-habit. Let's chat about how we'd like to contribute suggestions to PRs.

lightmedium commented 10 years ago

OMG you can DO that!? That's AWESOME! I guess it's the little things that move me :) We had a freakishly bastardized version of Stash at Chase, and it made it almost impossible to collaborate on pull requests.

lightmedium commented 10 years ago

...and good call on the view layer stuff. What about images? Do you see a need to keep assets/images, or should we just remove the whole assets directory?

ztbrown commented 10 years ago

Yea, I'd kill the assets directory entirely. And we should un-configure precompile assets as well (and any other Rails convention stuff regarding view that I'm not thinking of). Maybe we also whack any gem that is viewcentric like sass-rails. Not that that ALL needs to happen on this branch.

lightmedium commented 10 years ago

yep. let me know if you make any progress on that (and what you do) because I've never had to disable those things after the app was created.

ztbrown commented 10 years ago

I will make an issue for that.

This PR looks good and closes #2, so I'd put in a vote for :ship:. Let's wait to hear from @baroldgene @dswingle and @BDS1400 if possible.

lightmedium commented 10 years ago

sure, no rush. I'm going to look into OmniAuth now.

lightmedium commented 10 years ago

(and by "now" I mean maybe at lunch, but definitely later this evening)

ztbrown commented 10 years ago

Cool. Actually, this is missing one other thing that is mentioned in #2, Users should have a phone number. Could you throw that into the migration, as an attribute on the model and create a new assertion in the spec?

ztbrown commented 10 years ago

:+1:

lightmedium commented 10 years ago

One more commit and I'm done... for now.

lightmedium commented 10 years ago

So, how do we run the tests?

ztbrown commented 10 years ago 
$ rspec spec
lightmedium commented 10 years ago

I'm confused. When I created this pull request last night (admittedly two bourbons into the evening), did it create this issue at the same time?

ztbrown commented 10 years ago

Yea. It's an annoying implementation, IMO. A pull request creates a new issue (in this case #8). The issue we created that you are closing with this PR is #2.

So, we populate the issue backlog with a bunch of new issues and then reference those issue numbers in our PRs like you've done here. And in our comments, we can say things like Closes #2 and when you merge, the issue will auto-close.

lightmedium commented 10 years ago

I can live with the "duplicated" issues in exchange for how tightly integrated everything is with the core SCM features.

baroldgene commented 10 years ago

:thumbsup: