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

Joe M - Chitter #234

Open
wants to merge 10 commits 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Expand Up @@ -6,6 +6,7 @@ ruby '3.0.2'

gem 'pg'
gem 'sinatra'
gem 'webrick'

group :test do
gem 'capybara'
Expand Down
3 changes: 3 additions & 0 deletions Gemfile.lock
Expand Up @@ -79,11 +79,13 @@ GEM
unicode-display_width (>= 1.1.1, < 3)
tilt (2.0.10)
unicode-display_width (2.0.0)
webrick (1.7.0)
xpath (3.2.0)
nokogiri (~> 1.8)

PLATFORMS
x86_64-darwin-20
x86_64-darwin-21

DEPENDENCIES
capybara
Expand All @@ -93,6 +95,7 @@ DEPENDENCIES
simplecov
simplecov-console
sinatra
webrick

RUBY VERSION
ruby 3.0.2p107
Expand Down
17 changes: 17 additions & 0 deletions README.md
Expand Up @@ -34,18 +34,25 @@ You should see 1 passing test.

Choose a reason for hiding this comment

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

I found this framework really helpful when writing my README: https://github.com/makersacademy/course/blob/main/pills/readmes.md

## User stories


```
As a Maker
So that I can see what people are doing
I want to see all the messages (peeps)
in a browser
```


![alt text](story1diagram.png "See all messages diagram")

Choose a reason for hiding this comment

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

I really like the diagrams, they're super helpful for visualising the data flow in your program

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree! It's always a good idea to diagram things out.

One thing I would note is that in these diagrams you've included both app.rb and peeps.rb under the Controller and under model you have the database itself.

That's not quite how the MVC model is meant to be understood. The database isn't really a part of the MVC model, the MVC model only covers the code you write:

  • The Model part refers to the code you write that interacts with the database (peeps.rb) and that provides a way to represent the data once you've retrieved it from the database (one instance of the Peep class represents a single row of the DB)
  • As your diagram already suggests, the Controller part refers to the code you write that sets up routes and is the link between your View and your Model. It's just that in your codebase, this means that the Controller is really only the code in the app.rb file (in more complex codebases it could cover more files, but there's no need to worry about that at this point in the course!)


```
As a Maker
So that I can let people know what I am doing
I want to post a message (peep) to chitter
```
# Second Story diagram

![alt text](story2diagram.png "Post a message (peep) diagram")

```
As a Maker
Expand All @@ -54,6 +61,13 @@ I want to see the date the message was posted
```
(Hint the database table will need to change to store the date too)

# To do - Date of message

* to faciltate time of peep add column to the databases(dev and test)
* Update Readme with how to set up database tables to include date and time column
* add date and time to tests
* working through the tests, amend the app.rb and new peep files to capture the date and time of peeps

```
As a Maker
So that I can easily see the latest peeps
Expand All @@ -64,3 +78,6 @@ As a Maker
So that I can find relevant peeps
I want to filter on a specific keyword
```



25 changes: 25 additions & 0 deletions app.rb
@@ -1,9 +1,34 @@
require 'sinatra/base'
require './lib/peeps'
require 'time'

class Chitter < Sinatra::Base
get '/test' do
'Test page'
end
Comment on lines 6 to 8
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). It keeps everything nice and clean :)


get '/' do
@peeps = Peeps.all
p @peeps
erb :'index'
end

get '/new' do
erb :'new'
end

post '/' do
message = params['message']
p params['message']
time = Time.now.httpdate
connection = PG.connect(dbname: 'chitter')

Choose a reason for hiding this comment

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

One next step here would be to refactor this code into your Peeps class, similar to this step from the Bookmark Manager challenge: https://github.com/makersacademy/course/blob/main/apprenticeships_bookmark_manager/walkthroughs/10.md#refactoring

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that would be a good next step. It would allow you to test this code in a unit test just like you tested the Peep.all method in peeps_spec.rb.
It would also make the separation between Controller and Model that I talked about above clearer.

connection.exec("INSERT INTO peeps (message, posted) VALUES('#{message}', '#{time}')")
redirect '/'
end





run! if app_file == $0
end
2 changes: 1 addition & 1 deletion db/migrations/01_create_chitter_table.sql
@@ -1 +1 @@
CREATE TABLE peeps(id SERIAL PRIMARY KEY, message VARCHAR(60));
CREATE TABLE peeps(id SERIAL PRIMARY KEY, message VARCHAR(60), posted VARCHAR(60));

Choose a reason for hiding this comment

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

I'm not sure I 100% follow their reasoning, but Makers advise adding updates to the database table as a separate migration file: https://github.com/makersacademy/course/blob/main/apprenticeships_bookmark_manager/walkthroughs/11.md#updating-the-bookmarks-table

Also, depending on how you record the date / time, you could use a different SQL data type for the posted column: https://www.w3schools.com/sql/sql_datatypes.asp

Copy link
Contributor

Choose a reason for hiding this comment

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

Some good links there @olliefreeman94

The reason for a separate migration file is that in a real system, you would want to be able to run the migration separately from the initial creation of the table, as a DB will typically throw an error if you try to create a table that doesn't exist. If all the migrations are in the same file, they will never run because the error will happen before we even get to them.

Copy link
Contributor

Choose a reason for hiding this comment

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

When we want to add a field to a new table, we typically want to use an ALTER TABLE statement rather than create a brand new table. Think about what would happen -- all of the existing peeps would no longer be visible on the site because the new table doesn't contain them. The link @olliefreeman94 sent above has a good example of that.

Binary file added diagram.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
18 changes: 18 additions & 0 deletions lib/peeps.rb
@@ -0,0 +1,18 @@

require 'pg'
require 'time'

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

result = connection.exec('SELECT message, posted FROM peeps')
# binding.irb
result.map { |peep| peep }

Choose a reason for hiding this comment

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

This part of the Bookmark Manager walkthrough outlines wrapping the database data in an instance of the class: https://github.com/makersacademy/course/blob/main/apprenticeships_bookmark_manager/walkthroughs/11.md#wrapping-returned-data


end
end
Binary file added public/chitter.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 11 additions & 0 deletions spec/features/new_peep_spec.rb
@@ -0,0 +1,11 @@
# in spec/features/creating_bookmarks_spec.rb

feature 'Post a new peep' do
scenario 'A user can post a new peep to chitter' do
visit('/new')
fill_in('message', with: 'hey peeps. just a peep to say I am posting a new peep. so peep this!')
click_button('post your peep')

expect(page).to have_content 'hey peeps. just a peep to say I am posting a new peep. so peep this!'
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Good feature test!

end
15 changes: 15 additions & 0 deletions spec/features/view_peeps_spec.rb
@@ -0,0 +1,15 @@
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, 'Just completed SQL Zoo. It was fun to do!');")
connection.exec("INSERT INTO peeps VALUES(2, 'Testing adding chitter messages');")
connection.exec("INSERT INTO peeps VALUES(3, 'testing chitter.');")

visit('/')
expect(page).to have_content "Just completed SQL Zoo. It was fun to do!"
Copy link
Contributor

Choose a reason for hiding this comment

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

💥

expect(page).to have_content "Testing adding chitter messages"
expect(page).to have_content "testing chitter."
end
end
28 changes: 28 additions & 0 deletions spec/peeps_spec.rb
@@ -0,0 +1,28 @@
# in spec/peeps_spec.rb

require 'peeps'

describe Peeps do
describe '.all' do
it 'returns all peeps' do
connection = PG.connect(dbname: 'chitter_test')

# Add the test data
connection.exec("INSERT INTO peeps VALUES(1, 'Just completed SQL Zoo. It was fun to do!');")
connection.exec("INSERT INTO peeps VALUES(2, 'Testing adding chitter messages');")
connection.exec("INSERT INTO peeps VALUES(3, 'testing chitter.');")


peeps = Peeps.all

expect(peeps).to include "Just completed SQL Zoo. It was fun to do!"
expect(peeps).to include "Testing adding chitter messages"
expect(peeps).to include "testing chitter."
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice and thorough!
Even more thorough: checking that peeps has the right length -- what if some duplicate messages had crept in somehow?

end
end
end





2 changes: 1 addition & 1 deletion spec/spec_helper.rb
Expand Up @@ -29,7 +29,7 @@
])
SimpleCov.start

ENV['RACK_ENV'] = 'test'
# ENV['RACK_ENV'] = 'test'
ENV['ENVIRONMENT'] = 'test'

# Bring in the contents of the `app.rb` file. The below is equivalent to: require_relative '../app.rb'
Expand Down
Binary file added story1diagram.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added story2diagram.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions views/index.erb
@@ -0,0 +1,7 @@
<ul>
<% @peeps.each do |peep| peep['message'] %>
<li><%= peep %></li>

Choose a reason for hiding this comment

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

This part of the Bookmark Manager walkthrough outlines updating the view to show different properties of the Peeps object: https://github.com/makersacademy/course/blob/main/apprenticeships_bookmark_manager/walkthroughs/11.md#showing-title-in-the-bookmarks-view

<% end %>
</ul>

<img src='/chitter.png' />

Choose a reason for hiding this comment

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

The logo is a really nice addition!

8 changes: 8 additions & 0 deletions views/new.erb
@@ -0,0 +1,8 @@
<!-- inside views/new.erb -->

<form action = "/" method = "post" />
<input type="text" name="message" />
<input type="submit" value="post your peep" />
</form>

<img src='/chitter.png' />