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

Work In Progress Chitter app #2172

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

tomcarmichael
Copy link

Began implementation of home page, using sessions to display a form to create new peep only if the user is logged in. Began implementation of login and register pages.

Copy link

@toppy007 toppy007 left a comment

Choose a reason for hiding this comment

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

I know this is a work in progress so its a bit messy at the moment. looks like your got a nice structure of recording inputs and then requesting the data from the db. looks like your ready to begin displaying the required information as required.

app.rb Outdated
end

get '/' do
@all_peeps = PeepRespository.new.all_by_rev_date_order_with_author

Choose a reason for hiding this comment

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

method names could be shorter

app.rb Outdated
end

get '/login' do
if session[:user_id]

Choose a reason for hiding this comment

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

think about developing a controller method/class.

it "displays all peeps in reverse chronological order" do
response = get('/')
expect(response.status).to eq(200)
expect(response.body).to match(/Big Brother is watching you @wsmith[\s\S]*@wsmith & @smhanna - this is jam hot, this is jam hot[\s\S]*We shall meet in the place where there is no darkness/)

Choose a reason for hiding this comment

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

not sure I agree with such a large expects comment.
maybe break this up.

Choose a reason for hiding this comment

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

is it better you make a request for the data then sort it accordingly.

spec/seeds.sql Outdated

Choose a reason for hiding this comment

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

I would create a file called seeds to store all my database information with the spec.
e.g put chitter.sql and chitter_test.sql in the same location.

@@ -0,0 +1,3 @@
class User
attr_accessor :id, :name, :username, :email, :password

Choose a reason for hiding this comment

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

from the Peeps table I can see you have the columns user and user_id, then in Users table you have the column username. reckon you can drop a few of these columns as the project progresses. I'm guilty of the same thing.

if stored_password == submitted_password
return { success: true, username: user.username, user_id: user.id }
else
return { success: false, reason: "incorrect password "}

Choose a reason for hiding this comment

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

I like the reason statement. will take this for my own project.

schema_recipe.md Outdated

Choose a reason for hiding this comment

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

well planned out. might be worth adding a .md file to the seeds db file i suggested adding with some of the info below in it.

expect(repo.all.last.name).to eq "John Smith"
expect(repo.all.last.email).to eq "[email protected]"
end
xit "fails to add them given any missing info" do

Choose a reason for hiding this comment

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

look at developing a validation method to run on the inputs incorporating a conditional statement to decide to run the create new user method.

def invalid_request_parameters?
return true if params[:title].nil? || params[:title].empty? ||
params[:content].nil? || params[:content].empty?
false
end

Choose a reason for hiding this comment

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

This is quite cool! im not sure how dynamic HTML is at controlling conditional statements will be interesting to get your take on it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants