Joe M - Chitter #234

Joemusarurwa · 2022-07-15T10:29:13Z

Joe Musarurwa

User stories

Please list which user stories you've implemented (delete the ones that don't apply).

User story 1: "I want to see all the messages (peeps) in a browser"
User story 2: "I want to post a message (peep) to chitter"
User story 3: "I want to see the date the message was posted"
User story 4: "I want to see a list of peeps in reverse chronological order"
User story 5: "I want to filter on a specific keyword"

README checklist

Does your README contains instructions for

how to install,
how to run,
and how to test your code?

Here is a pill that can help you write a great README!

… still 1 failing test

…rted into table. Format needs work. Not pretty

olliefreeman94

I wasn't able to test the code locally, so haven't been able to look at everything outlined here: https://github.com/makersacademy/course/blob/main/pills/code_review_checklist.md

The code is really clear, easy to follow, and uses the MVC paradigm well.

olliefreeman94 · 2022-07-18T09:30:56Z

README.md

 ```
 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")


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

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!)

olliefreeman94 · 2022-07-18T09:34:18Z

app.rb

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


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

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.

olliefreeman94 · 2022-07-18T09:35:51Z

lib/peeps.rb

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


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

olliefreeman94 · 2022-07-18T09:37:35Z

views/index.erb

+  <% end %>
+</ul>
+
+<img src='/chitter.png' />


The logo is a really nice addition!

olliefreeman94 · 2022-07-18T09:38:43Z

README.md

@@ -34,18 +34,25 @@ You should see 1 passing test.



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

olliefreeman94 · 2022-07-18T09:42:57Z

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));


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

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.

olliefreeman94 · 2022-07-18T09:46:08Z

views/index.erb

@@ -0,0 +1,7 @@
+<ul>
+  <% @peeps.each do |peep| peep['message'] %>
+    <li><%= peep %></li>


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

siellsiell

Hey Joe,

Well done on completing the first two user stories. The diagrams were a great addition. I left a few more detailed comments below. I would suggest using this week where you'e building another database app to revisit some of the ideas of Database week, namely:

the difference between Model and Controller
how we can update existing tables in a database using migrations

Using Makersbnb as a way to revisit these topics would be a great use of this week. Makersbnb is intentionally an app quite similar to the one's you've built so far so that you can consolidate your learning from the previous weeks. The only new thing is the teamwork aspect.

If you have questions about anything I commented on, feel free to ask me here in the comments or on Slack!

Simo

siellsiell · 2022-07-19T16:17:56Z

README.md

 ```
 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")


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!)

siellsiell · 2022-07-19T16:18:26Z

app.rb

  get '/test' do
    'Test page'
  end


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 :)

siellsiell · 2022-07-19T16:20:06Z

app.rb

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


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.

siellsiell · 2022-07-19T16:20:37Z

spec/features/new_peep_spec.rb

+    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


Good feature test!

siellsiell · 2022-07-19T16:21:06Z

spec/features/view_peeps_spec.rb

+    connection.exec("INSERT INTO peeps VALUES(3, 'testing chitter.');")
+
+    visit('/')
+    expect(page).to have_content "Just completed SQL Zoo. It was fun to do!"


siellsiell · 2022-07-19T16:24:46Z

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));


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.

siellsiell · 2022-07-19T16:26:15Z

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));


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.

siellsiell · 2022-07-19T16:33:10Z

spec/peeps_spec.rb

+
+      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."


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

Joemusarurwa added 10 commits July 15, 2022 11:25

added diagram

f805bc8

testing viewing all peeps

ebefd7a

tested first story, added diagram

ef8c59a

amended diagram

ee1c196

added image for site, and button to post new peeps. web app works but…

c09557f

… still 1 failing test

add post mesage diagram. added to readme.

c503e40

added To do list for datae and time of peep

c22c57f

corrected typo on readme

2c312a2

amended readme

71508fb

new peep time stamp now populates in DB. Using Time.now.httpdate inse…

298ef53

…rted into table. Format needs work. Not pretty

olliefreeman94 reviewed Jul 18, 2022

View reviewed changes

siellsiell reviewed Jul 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Joe M - Chitter #234

Joe M - Chitter #234

Joemusarurwa commented Jul 15, 2022 •

edited

olliefreeman94 left a comment

olliefreeman94 Jul 18, 2022

siellsiell Jul 19, 2022

olliefreeman94 Jul 18, 2022

siellsiell Jul 19, 2022

olliefreeman94 Jul 18, 2022

olliefreeman94 Jul 18, 2022

olliefreeman94 Jul 18, 2022

olliefreeman94 Jul 18, 2022

siellsiell Jul 19, 2022

olliefreeman94 Jul 18, 2022

siellsiell left a comment

siellsiell Jul 19, 2022

siellsiell Jul 19, 2022

siellsiell Jul 19, 2022

siellsiell Jul 19, 2022

siellsiell Jul 19, 2022

siellsiell Jul 19, 2022

siellsiell Jul 19, 2022

siellsiell Jul 19, 2022

		@@ -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));

Joe M - Chitter #234

Are you sure you want to change the base?

Joe M - Chitter #234

Conversation

Joemusarurwa commented Jul 15, 2022 • edited

Joe Musarurwa

User stories

README checklist

olliefreeman94 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siellsiell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Joemusarurwa commented Jul 15, 2022 •

edited