-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
base: main
Are you sure you want to change the base?
Ongoing development #2184
Conversation
Added diagram for current state
Added diagram for current state
There was a problem hiding this 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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" } |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
No description provided.