sc0v / binder

The Spring Carnival managment application built for Carnegie Mellon Spring Carnival.
16 stars 19 forks source link

Added Dependency Tests for Participant, Coverage for Other Models & Bugfix #237

Closed AkshayGoradia closed 7 years ago

AkshayGoradia commented 7 years ago

Added dependency tests for participant model, coverage for other models and cart bug fix in which items were being duplicated.

AkshayGoradia commented 7 years ago

@pkoenig10 Do you know why these Travis CI builds failed? Not sure why it didn't find a schema.rb file.

pkoenig10 commented 7 years ago

It looks like you've (I'm assuming accidentally) removed that file in this PR.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 15.371% when pulling eced48e22499c2b8ddbb3b934b5a29d37c6b82f2 on ak/bugs into f21dba7d1e3bb20ff393c15f46abf2fc738bd5a2 on 17/goat/main.

pkoenig10 commented 7 years ago

@AkshayGoradia the schema file you added back isn't the same as the one removed. Can you just modify the commit in which it was removed to revert any changes to this file?

AkshayGoradia commented 7 years ago

Woops, added the schema file back – thank you!

@pkoenig10 looks like I'm failing the push build because it can't find the factory: organization_status. I thought I was supposed to delete it because Bruce created and merged in his own organization_status factory a while back (Travis CI already gave us an error before when it said the factory organization_status already existed). Thanks so much for your help!

pkoenig10 commented 7 years ago

It looks like your branch branched from your team branch before his organization status commits were commited. You should rebase your branch on top of the team branch before you commit. Just a friendly reminder to coordinate when multiple people are working on the same thing to make sure we don't break things as a result of merging.

I'm going to disable merging PRs for the whole repo, instead only allowing squash and merge. This makes our commit history cleaner and prevents (all too frequent) errors that result from merging.

coveralls commented 7 years ago

Coverage Status

Coverage increased (+0.6%) to 15.371% when pulling 48a51f1d0664a881eb82cd1666db74d664ca061f on ak/bugs into f21dba7d1e3bb20ff393c15f46abf2fc738bd5a2 on 17/goat/main.

ChaseBro commented 7 years ago

I don't think that merge commits in master cause that many issues and would rather have the history if that's cool? I'm cool not allowing rebase & merge as I think that can cause weirdness (and makes it harder to revert), but in general I like having the full commit history

pkoenig10 commented 7 years ago

The main motivation is that usually PRs are filled with a bunch of small fix commits which, IMO, make the history harder to parse. This PR is an excellent example. Not to mention all the merge commits.

Of course there are some PRs with multiple "important" commits, but I would argue that if those features are so important they deserve to be preserved in the history, they should be separate PRs.

pkoenig10 commented 7 years ago

But you're right, I suppose it depends on the situation. For features and bugfixes being contributed to the team branch, we should use squash and merge. But when merging into master we definitely don't want to squash to keep the teams work history.