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.

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

User stories 1 and 2 completed (temporary solution to user story 4) #237

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ ruby '3.0.2'

gem 'pg'
gem 'sinatra'
gem 'sinatra-contrib'

group :test do
gem 'capybara'
Expand Down
3 changes: 3 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ GEM
mini_mime (1.1.1)
mustermann (1.1.1)
ruby2_keywords (~> 0.0.1)
nokogiri (1.12.3-arm64-darwin)
racc (~> 1.4)
nokogiri (1.12.3-x86_64-darwin)
racc (~> 1.4)
parallel (1.20.1)
Expand Down Expand Up @@ -83,6 +85,7 @@ GEM
nokogiri (~> 1.8)

PLATFORMS
arm64-darwin-21
x86_64-darwin-20

DEPENDENCIES
Expand Down
17 changes: 17 additions & 0 deletions app.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,26 @@
require 'sinatra/base'
require 'sinatra/reloader'
require_relative 'lib/peep'

class Chitter < Sinatra::Base
configure :development do
register Sinatra::Reloader
end

get '/test' do
'Test page'
end
Comment on lines 10 to 12
Copy link
Contributor

Choose a reason for hiding this comment

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

It's always a good idea to remove test code from code you submit for code review (anywhere, not just at Makers). Just keeps things clean and easy to follow.


get '/' do
@all_peeps = Peep.all
erb(:home)
end

post '/' do
Peep.create(params[:message])
redirect("/")
end


run! if app_file == $0
end
24 changes: 24 additions & 0 deletions lib/peep.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
require 'pg'

class Peep
def self.all
if ENV['ENVIRONMENT'] == 'test'
Copy link
Author

Choose a reason for hiding this comment

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

There could be a method for this, in order to avoid repeating code!

Copy link
Contributor

Choose a reason for hiding this comment

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

Good observation! If you hadn't commented on it yourself, I would have 😁

connection = PG.connect(dbname: 'chitter_test')
else
connection = PG.connect(dbname: 'chitter')
end

result = connection.exec "SELECT * FROM peeps"
return result.map{ |elem| elem["message"]}.reverse
Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually use the database query to sort the peeps in the right order and save the extra reversal step: https://www.postgresql.org/docs/current/queries-order.html
If you had a lot of peeps, this could improve perfomance quite a lot.

end

def self.create(peep)
if ENV['ENVIRONMENT'] == 'test'
connection = PG.connect(dbname: 'chitter_test')
else
connection = PG.connect(dbname: 'chitter')
end

connection.exec "INSERT INTO peeps (message) VALUES('#{peep}')"
end
end
10 changes: 10 additions & 0 deletions spec/features/create_peep_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
feature 'Creating a new peep' do
Copy link
Author

Choose a reason for hiding this comment

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

Test coverage is good, but the types of input (try invalids too?) aren't being tested. Only one type of input is being tested here...

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point. To make your application more robust, you'd want to make sure you caught invalid inputs and test that too. In the context of this challenge, it's a good enough feature test though :)

scenario 'A user can write and send new Peeps' do

visit('/')
fill_in('message', with: "This is a new Peep!")
click_button('Submit')

expect(page).to have_content "This is a new Peep!"
end
end
19 changes: 19 additions & 0 deletions spec/features/peeps_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
require 'pg'
require 'peep'

feature 'Viewing Peeps' do
scenario 'A user can see Peeps' do
connection = PG.connect(dbname: 'chitter_test')

# Add the test data
Peep.create('New Peep 1!')
Peep.create('Yoyoyo!')
Peep.create('http://www.google.com')

visit('/')

expect(page).to have_content "New Peep 1!"
expect(page).to have_content "Yoyoyo!"
expect(page).to have_content "http://www.google.com"
end
end
Copy link
Author

Choose a reason for hiding this comment

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

Always include a new line at the end of a code file!

28 changes: 28 additions & 0 deletions spec/homepage_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
require 'pg'
require 'peep'

describe '.all' do
it 'returns all peeps in reverse chronological order' do
connection = PG.connect(dbname: 'chitter_test')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you actually need this line?


# Add the test data
Peep.create('New Peep 1!')
Peep.create('Yoyoyo!')
Peep.create('http://www.google.com')

peeps = Peep.all

expect(peeps).to include('New Peep 1!')
expect(peeps).to include('Yoyoyo!')
expect(peeps).to include('http://www.google.com')
Comment on lines +15 to +17
Copy link
Contributor

Choose a reason for hiding this comment

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

As you're using include, this doesn't actually test that the peeps are in the right order.

end
end

describe '.create' do
it 'adds peeps to the database' do
connection = PG.connect(dbname: 'chitter_test')
Peep.create("Peep peep!")
peeps = Peep.all
expect(peeps).to include('Peep peep!')
end
end
10 changes: 10 additions & 0 deletions views/home.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<div style="text-align: center">
<h1>Chitter:</h1>

<form action="/" method="post">
<input name="message" type="text" placeholder="Write a new Peep..."/>
<button value="Submit" type="submit">Peep!</button>
</form>

<p><%= @all_peeps.join('/n') %></p>
Copy link
Author

Choose a reason for hiding this comment

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

This wouldn't work - ('/n') would just show up as a string. Maybe "/n" (full quotation marks) would work?

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the HTML equivalent of a line break? That's the thing to research.

</div>