Skip to content

Carnival Creation #294

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.

Sign up for GitHub

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

Jump to bottom
Open
Aklaran wants to merge 44 commits into
base: master
Choose a base branch
from
Open

Carnival Creation #294

Aklaran wants to merge 44 commits into from

Conversation

@Aklaran
Copy link
Contributor

@Aklaran Aklaran commented Mar 29, 2019

No description provided.

@Aklaran Aklaran requested a review from sclark March 29, 2019 20:55
@coveralls
Copy link

coveralls commented Mar 29, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-2.1%) to 12.927% when pulling db795a7 on carnival-creation into 9cb5bd0 on master.

sclark
sclark approved these changes Mar 31, 2019
View reviewed changes
Copy link
Member

@sclark sclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the test isn't passing, but once it does, lgtm!

app/models/csv_updater.rb
@@ -0,0 +1,389 @@
class CsvUpdater
Copy link
Member

@sclark sclark Mar 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be in models/ or can we put it in helpers?

Copy link
Contributor Author

@Aklaran Aklaran Mar 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be put into helpers. I'll make this change

test/controllers/carnival_creation_controller_test.rb
@@ -0,0 +1,14 @@
require 'test_helper'

class CarnivalCreationControllerTest < ActionController::TestCase
Copy link
Member

@sclark sclark Mar 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test!

@timparenti timparenti mentioned this pull request Apr 4, 2019
Open
@timparenti timparenti mentioned this pull request Apr 26, 2019
Open
@timparenti
Copy link
Contributor

timparenti commented Apr 26, 2019 •
edited
Loading

@Aklaran, per today's meeting, you may be able to add to your documentation in-progress simple workarounds for #300.

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

Reviewers

1 more reviewer

@sclark sclark sclark approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Aklaran @coveralls @timparenti @sclark