Chitter solo challenge by olliefreeman94 · Pull Request #231 · makersacademy/chitter-challenge-apprenticeships

olliefreeman94 · 2022-07-15T09:35:56Z

Your name

Ollie Freeman

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!

Joemusarurwa · 2022-07-18T09:30:14Z

+    @view_all_button = 1
+    erb :'peeps/index'
+  end
+


clean and concise. I can follow the workflow from how the methods are structured.

Joemusarurwa · 2022-07-18T09:34:29Z

+- message character limit validation
+- cf. Bookmark Manager extensions,
+    e.g. update (inc. logging?), delete, user authentication
+- equivilent of Twitter #


good use of notes and to do to plan next steps

Joemusarurwa · 2022-07-18T09:36:37Z

+
+<form action="/peeps" method="post">
+  <input type="text" name="message" placeholder="Message"/>
+  <input type="hidden" name="date" value="<%=Date.today%>">


awesome way to get the time stamp with a hidden field

siellsiell

Hey Ollie,

This is a great solution to the challenge, really well done. Your testing in particular is very thorough, which is nice to see.

I've left a few more detailed comments below for things you could do in the future to make your code even better.

If you have any questions, feel free to ask in the comments on here or in a DM.

Simo

siellsiell · 2022-07-21T09:19:07Z

@@ -1,66 +1,42 @@
-## Chitter Challenge


Nice README, it's very clear.

siellsiell · 2022-07-21T09:19:23Z

  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)

siellsiell · 2022-07-21T09:19:59Z

+  get '/peeps/new' do
+    erb :'peeps/new'
+  end
+
+  get '/peeps/search' do
+    erb :'peeps/search'
+  end
+
+  get '/peeps/filtered' do
+    @peeps = Peep.filter(params[:keyword])
+    @view_all_button = 1
+    erb :'peeps/index'
+  end


You've shown good use of RESTful routes 👍🏽

siellsiell · 2022-07-21T09:21:36Z

+
+  get '/peeps/filtered' do
+    @peeps = Peep.filter(params[:keyword])
+    @view_all_button = 1


I saw further down that you're using this as a kind of boolean flag, in which case it would be better to make your intention explicit here and set this to true rather than one.

siellsiell · 2022-07-21T09:22:34Z

@@ -0,0 +1 @@
+ALTER TABLE peeps ADD COLUMN date DATE;


Separate migration file -- great. And DATE is an appropriate datatype to use. Another option would have been to use TIMESTAMP. That would give you flexibility in the future if it ever becomes necessary to show time information too.

siellsiell · 2022-07-21T09:33:35Z

+    expect(page).to have_content "Banana peep (2022-07-15)"
+    expect(page).to have_content "Another banana peep (2022-07-05)"


This is good but doesn't exclude the possibility that the other non-banana peeps are also on the page! How could you test for that?

siellsiell · 2022-07-21T09:34:57Z

@@ -0,0 +1,43 @@
+feature 'pathing between routes' do


This is very thorough, nice!

siellsiell · 2022-07-21T09:36:05Z

+
+    visit('/peeps')
+
+    expect(page).to have_content(/New peep.*Old peep/)


Nice way to test this.

siellsiell · 2022-07-21T09:37:15Z

+      expect(peeps.first).to be_a Peep
+      expect(peeps.first.id).to eq peep.id
+      expect(peeps.first.message).to eq("This is a peep!")
+      expect(peeps.first.date).to eq("2022-07-15")


Since the peeps array could have duplicates (due to a bug), it's also necessary to test that the other 2 peeps are the ones you expect.

siellsiell · 2022-07-21T09:55:26Z

+
+<form action="/peeps" method="post">
+  <input type="text" name="message" placeholder="Message"/>
+  <input type="hidden" name="date" value="<%=Date.today%>">


Using a hidden field works but it does mean that a malicious user could issue their own POST request and set a time of their choosing. The other pitfall is that the date will be generated at the time when this page is rendered rather than at the time when the user clicks submit, so at midnight boundaries it might end up being wrong.

For reasons like that it can be better to generate the date in your Model code just before you insert the data into the database.

Project setup

5531edd

olliefreeman94 marked this pull request as ready for review July 15, 2022 09:36

olliefreeman94 added 13 commits July 15, 2022 10:51

Add index page

9b5d95c

Refactor '/' index route to use ERB file

3778506

Implement '/peeps' route for viewing messages

249c22f

Refactor to use peeps.erb view

e26b9aa

Refactor '/peeps' route to use Peep class

79b7258

Refactor .all method to query database; add test data for each test

b11aa38

Implement '/peeps/new' route for adding new message

33c6b62

Refactor to wrap data in program objects

a731fd5

Update .add method to defend against SQL injection

8333709

Record date for peeps

3f40e0f

Implenment sorting by reverse chronological order

53068bf

Search by keyword

484224d

Refactot to use Date object, and hide date field in form

07af151

olliefreeman94 changed the title ~~Project setup~~ Chitter solo challenge Jul 17, 2022

Add pathing between routes

888c6e8

Joemusarurwa reviewed Jul 18, 2022

View reviewed changes

siellsiell reviewed Jul 21, 2022

View reviewed changes

		expect(page).to have_content "Banana peep (2022-07-15)"
		expect(page).to have_content "Another banana peep (2022-07-05)"


		visit('/peeps')

		expect(page).to have_content(/New peep.*Old peep/)

Conversation

olliefreeman94 commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Your name

User stories

README checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siellsiell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

olliefreeman94 commented Jul 15, 2022 •

edited

Loading