Skip to content

Carnival Creation#294

Open
Aklaran wants to merge 44 commits into
masterfrom
carnival-creation
Open

Carnival Creation#294
Aklaran wants to merge 44 commits into
masterfrom
carnival-creation

Conversation

@Aklaran

@Aklaran Aklaran commented Mar 29, 2019

Copy link
Copy Markdown
Contributor

No description provided.

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

coveralls commented Mar 29, 2019

Copy link
Copy Markdown

Coverage Status

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

@sclark sclark left a comment

Copy link
Copy Markdown
Member

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!

Comment thread app/models/csv_updater.rb
@@ -0,0 +1,389 @@
class CsvUpdater

Copy link
Copy Markdown
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
Copy Markdown
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

@@ -0,0 +1,14 @@
require 'test_helper'

class CarnivalCreationControllerTest < ActionController::TestCase

Copy link
Copy Markdown
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 commented Apr 26, 2019

Copy link
Copy Markdown
Contributor

@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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants