Skip to content
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

Carnival Creation #294

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

Carnival Creation #294

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 Outdated
@@ -0,0 +1,389 @@
class CsvUpdater
Copy link
Member

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

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

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

@sclark sclark sclark approved these changes

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.
4 participants
@Aklaran @coveralls @timparenti @sclark