Skip to content

Q Chitter Challenge - #2178

Open
SomthingInteresting wants to merge 17 commits into
makersacademy:mainfrom
SomthingInteresting:main
Open

Q Chitter Challenge #2178
SomthingInteresting wants to merge 17 commits into
makersacademy:mainfrom
SomthingInteresting:main

Conversation

@SomthingInteresting

Copy link
Copy Markdown

No description provided.

@umutbaykan umutbaykan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

sorry about the grammar and tone, we were in a rush! :)

Comment thread lib/peeps_repository.rb
Comment on lines +9 to +14
new_peep = Peep.new(
id: peep['id'].to_i,
content: peep['content'],
user_id: peep['user_id'].to_i,
created_at: peep['created_at']
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

new_peep = Peep.new
new_peep.id = peep['id'].to_i..
etc. etc.

Comment thread lib/peeps_repository.rb

new_peep
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

return result before the def all method finishes (ends)

Comment thread lib/peeps_repository.rb Outdated
# Debug statement
# puts "Peep created_at: #{new_peep.created_at.inspect}"

new_peep

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this needs to be removed

Comment thread spec/integration/chitter_app_spec.rb Outdated
"Hello, world!"
]

actual_peeps = response.body.scan(/<p class="peep__content">(.+)<\/p>/).flatten

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we need additional tests to check for format of peeps, as in they include the username, time etc..

Comment thread spec/integration/chitter_app_spec.rb Outdated

expected_username = "orangeman"
actual_username = response.body.scan(/<p class="peep__username">(.+)<\/p>/).flatten.first

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

maybe we can add one more test to check full format of one peep?

Comment on lines +4 to +8
def reset_tables
seed_sql = File.read('spec/seeds/chitter_seed.sql')
connection = PG.connect({ host: '127.0.0.1', dbname: 'chitter_test' })
connection.exec(seed_sql)
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same method happens in user repo tests, can we move those two to a singular location to have control from a single point?

Comment thread lib/users_repository.rb
email: user['email']
)
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

would be nice to see return result to clarify what it is returning

Comment on lines +1 to +2
TRUNCATE TABLE users, peeps, notifications RESTART IDENTITY;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

unrelated to this file, but would be good to see the create table sql file in here as well

require 'users'
require 'users_repository'

def reset_tables

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same comment as above

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants