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

15/07/22 #236

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 .rspec
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
--require spec_helper
--format documentation
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ gem 'pg'
gem 'sinatra'

group :test do
get 'timecop'
gem 'capybara'
gem 'rspec'
gem 'simplecov', require: false
Expand All @@ -17,3 +18,5 @@ end
group :development, :test do
gem 'rubocop', '1.20'
end

gem "timecop", "~> 0.9.5"
5 changes: 5 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 @@ -78,11 +80,13 @@ GEM
terminal-table (3.0.1)
unicode-display_width (>= 1.1.1, < 3)
tilt (2.0.10)
timecop (0.9.5)
unicode-display_width (2.0.0)
xpath (3.2.0)
nokogiri (~> 1.8)

PLATFORMS
arm64-darwin-21
x86_64-darwin-20

DEPENDENCIES
Expand All @@ -93,6 +97,7 @@ DEPENDENCIES
simplecov
simplecov-console
sinatra
timecop (~> 0.9.5)

RUBY VERSION
ruby 3.0.2p107
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ As a Maker
So that I can see when people are doing things
I want to see the date the message was posted
```
(Hint the database table will need to change to store the date too)
(Hint the database table will need to change to store the date too) - add an extra column

```
As a Maker
Expand Down
27 changes: 27 additions & 0 deletions app.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,36 @@
require 'sinatra/base'
require './lib/chitter'

class Chitter < Sinatra::Base
get '/test' do
'Test page'
end

get '/' do
erb :index
end

get '/peeps' do
@peeps = Peep.all
# @time = Peep.time
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 commented out code from code you submit for code review (anywhere, not just at Makers)

erb :peeps
end

post '/peeps' do
Peep.create(peep: params[:peep], timestamp: params[:timestamp])
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing the time in here is one way to do it but it's vulnerable to bugs. For example:

  • the user may click "submit" some time after the page was generated from your erb view, so the time of the peep will be inaccurate
  • the user can influence the time by sending their own POST requests and putting in a bogus time (e.g. to get their peep to show first even though it's old)

For such reasons, it's best to generate timestamps in your Model when you're about to add the peep to the database so that you're sure it will be accurate. I see that you were thinking along those lines in your commented out code in the chitter.rb. You were on the right track!

To control the time in tests, you can stub the return value of Time.now (see another comment further down).


# post '/bookmarks' do
# Bookmark.create(url: params[:url], title: params[:title])
# redirect '/bookmarks'
# end
# @time = Peep.time
@peeps = Peep.all
erb :peeps
end

get '/peeps/add' do
erb :"/peeps/add"
end

run! if app_file == $0
end
1 change: 1 addition & 0 deletions db/migrations/01_create_chitter_table.sql
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
-- To both chitter and chitter_test databases:
CREATE TABLE peeps(id SERIAL PRIMARY KEY, message VARCHAR(60));
2 changes: 2 additions & 0 deletions db/migrations/02_add_timestamp_to_chitter.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
-- To both chitter and chitter_test databases:
ALTER TABLE peeps ADD COLUMN timestamp VARCHAR(60);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice use of the timestamp column type!

35 changes: 35 additions & 0 deletions lib/chitter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
require 'pg'

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

result = connection.exec("SELECT * FROM peeps")
result.map { |peep| peep['message'] }
end

def self.create(peep:, timestamp:)
if ENV['ENVIRONMENT'] == 'test'
connection = PG.connect(dbname: 'chitter_test')
else
connection = PG.connect(dbname: 'chitter')
end
Comment on lines +16 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

You're using this same piece of code in more than one place in this file. How might you get rid of this duplication?


connection.exec("INSERT INTO peeps (message, timestamp) VALUES('#{peep}', #{timestamp})")
Comment on lines +2 to +22
Copy link

Choose a reason for hiding this comment

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

nice and easy to read

# connection.exec("INSERT INTO bookmarks (title, url) VALUES('#{title}', '#{url}') RETURNING id, url, title")
end

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

# connection.exec("INSERT INTO peeps (timestamp) VALUES('#{Time.now}')")
# end
end
8 changes: 8 additions & 0 deletions spec/features/adding_peeps_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
feature 'Viewing peeps' do
scenario 'viewing related timestamp' do
Copy link

Choose a reason for hiding this comment

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

It seems like this test is just for viewing the peeps rather than timestamps, as the expected outcome is just a lil peep peep. I'm guessing you were halfway through adapting an existing test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice observation @Amy-O ! I agree, right now this test is not testing what it says it does.

visit('/peeps/add')
fill_in('peep', with: 'Peep peep!')
click_button('Post')
expect(page).to have_content "Peep peep!"
end
end
6 changes: 6 additions & 0 deletions spec/features/homepage_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
feature 'Viewing homepage' do
scenario 'visiting homepage' do
visit('/')
expect(page).to have_content "Welcome to Chitter!"
end
end
8 changes: 8 additions & 0 deletions spec/features/timestamp_rspec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
feature 'Adding a peep' do
scenario 'adding a peep' do
visit('/peeps/add')
fill_in('peep', with: 'Peep peep!')
click_button('Post')
expect(page).to have_content ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be expecting the page to have "Peep peep!" as content?

end
end
26 changes: 26 additions & 0 deletions spec/features/viewing_peeps_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
require 'pg'
require './lib/chitter'

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

# # Add the test data
# connection.exec("INSERT INTO peeps VALUES(1, 'Peep one');")
# connection.exec("INSERT INTO peeps VALUES(2, 'Peep two');")
# connection.exec("INSERT INTO peeps VALUES(3, 'Peep three');")

# Bookmark.create(website: "http://www.makersacademy.com")
# Bookmark.create(website: "http://www.destroyallsoftware.com")
# Bookmark.create(website: "http://www.google.com")
Comment on lines +6 to +15
Copy link

Choose a reason for hiding this comment

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

I didn't think to add the bookmark stuff in comments for easier comparison! smart


Peep.create(peep: 'Peep one')
Peep.create(peep: 'Peep two')
Peep.create(peep: 'Peep three')
visit('/peeps')

expect(page).to have_content "Peep one"
expect(page).to have_content "Peep two"
expect(page).to have_content "Peep three"
end
end
1 change: 1 addition & 0 deletions spec/setup_test_database.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ def add_row_to_test_database
connection = PG.connect(dbname: 'chitter_test')
connection.exec("INSERT INTO peeps (message) values ('This is a peep!');")
end

6 changes: 6 additions & 0 deletions spec/spec_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
require 'rspec'
require 'simplecov'
require 'simplecov-console'
require 'timecop'

require_relative './setup_test_database'

Expand All @@ -41,6 +42,11 @@
RSpec.configure do |config|
config.before(:each) do
setup_test_database

# new_time = Time.local(2008, 9, 1, 12, 0, 0)
# Timecop.freeze(new_time)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you were trying to use Timecop to freeze the time during tests. Maybe it was tricky getting it to work? An alternative that doesn't require an additional gem is to use mocking, e.g.:

@now = Time.now
allow(Time).to receive(:now).and_return(@now)



end

config.after(:suite) do
Expand Down
1 change: 1 addition & 0 deletions views/index.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<h1> Welcome to Chitter! </h1>
5 changes: 5 additions & 0 deletions views/peeps.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<ul>
<% @peeps.each do |peep| %>
<li><%=peep%></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

All the rest of your code is there to fulfil user story 3 (showing the timestamp) -- did you run out of time or did you get blocked?

<% end %>
</ul>
5 changes: 5 additions & 0 deletions views/peeps/add.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
<form method="post" action="/peeps">
<input type="text" name="peep" placeholder="Add your thoughts" />
<input type="hidden"name="timestamp" value="<%=Time.now%>">
<input type="submit" value="Post" />
</form>