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

Ongoing development #2184

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

Ongoing development #2184

wants to merge 13 commits into from

Conversation

SunainaR
Copy link

No description provided.

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.

Hey Sunny. Thanks for letting me have a look at your project. It's good and great to see you got active record working! Brave direction to go so nice to see you took the challenge on. P.S I don't really know what I'm doing in these reviews!

First thing I would tackle is when I deployed the app.rb with rackup was being unable to pass the password conditional statement but I think thats a quick fix because I can see its in the database when I've changed the connection setting in the app.py

HTML is pretty limited in its style so don't worry! It made me smile to see three typeface families in your CSS file.

@@ -1,14 +1,42 @@
GEM
remote: https://rubygems.org/
specs:
activemodel (7.0.4.3)

Choose a reason for hiding this comment

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

OMG! this was a brave set to make. looking forward to you explaining it to me.


private
def password_encryption(password)
encrypted_password = BCrypt::Password.create(password)

Choose a reason for hiding this comment

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

Like how you've made the password method private.

## 5. Write SQL for seed files

psql -h 127.0.0.1 chitter_test < spec/seeds/table_creation.sql
psql -h 127.0.0.1 chitter_test < spec/seeds/seeds_chitter.sql

Choose a reason for hiding this comment

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

Couldn't find this file in the the repo!

require 'bcrypt'

# Need to edit database_connection later so this would work when deployed
set :database, { adapter: "postgresql", database: "chitter_test" }

Choose a reason for hiding this comment

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

Yeah noticed this! is this because you didn't want to fill up you deployed database with a load of rubbish when testing in rack-up?

not sure if this is best practice?

return erb(:login)
end

post '/login' do

Choose a reason for hiding this comment

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

I think you need to have another method in this section

decryption method (to take the encrypted password in the database and convert it back to the original password)

Dont worry the BCrypt gem does all the work just need to learn the method!

stored_password = BCrypt::Password.new(user.password)

not sure if that makes any sense or even if I'm correct tho but can't login when deploying the code on localhost!

@@ -0,0 +1,3 @@
# Active Record Model Class for peeps table
class Peep < ActiveRecord::Base

Choose a reason for hiding this comment

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

This looks like a game changer! nice one for going outside the comfy zone

@@ -0,0 +1,3 @@
# Active Record Model Class for users table
class User < ActiveRecord::Base

Choose a reason for hiding this comment

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

This looks like a game changer! nice one for going outside the comfy zone

@@ -0,0 +1,11 @@
body {
font-family: Verdana, Arial, Helvetica, sans-serif;

Choose a reason for hiding this comment

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

OMG, both my brothers are graphic designers! They would be going crazy! three different type families in one project! very debatable

Choose a reason for hiding this comment

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

Like how you've created a separate file system for the CSS. I should have done that

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