Subject editing #9
Conversation
…round image on user log in (workarround)
…sary, fixed spacing etc...
…sary, fixed spacing etc, solved merge issues
… styling to subjects main page
.rubocop_todo.yml
Outdated
| @@ -0,0 +1,110 @@ | |||
| # This configuration was generated by | |||
There was a problem hiding this comment.
there is no need to use .rubocop_todo.yml in new project. Main purpose for .rubocop_todo.yml is to make it easier to add rubocop in huge projects with many violations. Since you are starting almost from scratch, I would recommend to get rid of this file and do rubocop autofix instead (rubocop -a )
There was a problem hiding this comment.
removed .rubocop_todo.yml from project source, form now on I won't use it in my fresh new projects
Gemfile
Outdated
| gem 'devise' | ||
| gem 'omniauth-google-oauth2' | ||
| gem 'omniauth-twitter' | ||
| gem 'pry' |
There was a problem hiding this comment.
pry, capybara', factory_girl_rails, rspec-railsshould go to:develoment, :test` group. There is no point of having them in production environment
There was a problem hiding this comment.
fixed, next time will try not to be absent-minded
Gemfile
Outdated
| gem 'rails_12factor' | ||
| end | ||
|
|
||
| group :test do |
There was a problem hiding this comment.
if you do not use this group than it's safe to remove it
| @@ -0,0 +1,3 @@ | |||
| # Place all the behaviors and hooks related to the matching controller here. | |||
There was a problem hiding this comment.
as I understand this file was generated by rails g scaffold, but there is no point of having it in repo since it's empty
There was a problem hiding this comment.
added to .gitignore, removed from repo
| @@ -0,0 +1,3 @@ | |||
| # Place all the behaviors and hooks related to the matching controller here. | |||
There was a problem hiding this comment.
as I understand this file was generated by rails g scaffold, but there is no point of having it in repo since it's empty
There was a problem hiding this comment.
added to .gitignore, removed from repo
spec/models/user_spec.rb
Outdated
| describe User do | ||
| describe "validating user information" do | ||
| it "has a valid factory" do | ||
| FactoryGirl.create(:user).should be_valid |
There was a problem hiding this comment.
you can create new records with shorter form create(:user)
to do so, you need to include this in your rails_helper.rb:
https://github.com/thoughtbot/factory_bot/blob/master/GETTING_STARTED.md#rspec
There was a problem hiding this comment.
Good advise, fixed that as well, and changed FactoryGirl deprecated version to FactoryBot
app/models/user.rb
Outdated
| end | ||
|
|
||
| # Returns all users, there emails have a match | ||
| def self.find_mathing_letter(letter) |
There was a problem hiding this comment.
it this method used anywhere except tests?
There was a problem hiding this comment.
In the future, when the user is going to look for other users and their emails (issue #6 ), this function should come in handy, right now I'm not using it, just practicing to write some tests.
spec/models/user_spec.rb
Outdated
| describe User do | ||
| describe "validating user information" do | ||
| it "has a valid factory" do | ||
| FactoryGirl.create(:user).should be_valid |
There was a problem hiding this comment.
also since you are testing build in validators, I do not see a point of testing them
There was a problem hiding this comment.
mostly I used it for learning purposes, my model is almost empty, got you 👍
spec/models/user_spec.rb
Outdated
|
|
||
|
|
||
| describe User do | ||
| describe "validating user information" do |
There was a problem hiding this comment.
in most cases describe part should describe which method are you testing and context part should describe conditions in which tests are run. For example:
describe User do
describe '#valid?' do # instance methods starts with '#' and class methods starts with '.' (dot)
context 'when email is missing' do
#...
end
end
describe '.find_mathing_letter' do
context 'when there is a match'
context 'when there is no match'
end
end
There was a problem hiding this comment.
thank you, good advise, would try keep that in mind next time
spec/models/user_spec.rb
Outdated
| "locale"=>"en"}}}.to_json | ||
|
|
||
| parsedJson = JSON.parse(jsonResponseString, object_class: OpenStruct) | ||
| expect(User.from_omniauth(parsedJson)).not_to be_falsey |
There was a problem hiding this comment.
you could use double here and reduce test setup dramaticly, For example:
auth_object = double(
provider: 'google',
info: double(email: user.email),
# ... more methods which are used inside from_omniauth
)
expect(User.from_omniauth(auth_object)).not_to be_falsey
There was a problem hiding this comment.
fixed, removed instance variable and replaced with double
|
That’s a decent review @povilasjurcys 👍 |
app/models/subject.rb
Outdated
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| class Subject < ApplicationRecord | |||
| validates :title, presence: true, length: { minimum: 5 } | |||
| validates :body, presence: true | |||
| validates :body, presence: true, length: {minimum: 20} | |||
There was a problem hiding this comment.
can you integrate https://codeclimate.com/ in your project. It will give you comments about inconsistent style instantly
| end | ||
| end | ||
|
|
||
| let(:user) { FactoryBot.create(:user) } |
There was a problem hiding this comment.
keep all let and before statements in one place so it will be easier to track all context changes
There was a problem hiding this comment.
Fixed, if I correctly understood you, also fixed the FactoryBot to a shorter version (which you pointed out before)
Gemfile
Outdated
| @@ -72,6 +72,8 @@ group :development, :test do | |||
| gem 'rubocop', require: false | |||
| gem 'rubocop-rspec', require: false | |||
| gem 'shoulda-matchers', git: 'https://github.com/thoughtbot/shoulda-matchers.git', branch: 'rails-5' | |||
| gem 'rails-controller-testing' | |||
There was a problem hiding this comment.
i think this gem should not belong to :development group
| end | ||
|
|
||
| context 'when user fails to save subject' do | ||
| it 'title is too short' do |
There was a problem hiding this comment.
tests is your documentation so it would be nice if you could read them like a normal english sentences. For example this particular test will be converted to sentence:
SubjectsController GET #new
when user fails to save subject
title is too short
which is not very human readable. On the other hand you have quite fluent tests in 47 or 52 lines:
SubjectsController GET #new
when user saves subject
creates the subject and redirect to subject view
adds subject in subjects table`
try running your specs with documentation formatter:
rspec -f documentation
and you will see more examples
There was a problem hiding this comment.
Oh, okay, I will keep that in mind next time, thanks for good advice!
| @@ -0,0 +1,8 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class Subject < ApplicationRecord | |||
There was a problem hiding this comment.
Style/Documentation: Missing top-level class documentation comment.
There was a problem hiding this comment.
we try to make rubocop happy. If you disagree with rubocop then you have two ways of solving this problem:
a) add comment # rubocop:disable Style/Documentation
b) disable Style/Documentation rule in .rubocop.yml file for project or directory
app/models/user.rb
Outdated
| validates :email, uniqueness: true | ||
| has_many :subjects | ||
|
|
||
| def self.from_omniauth(auth) |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for from_omniauth is too high. [20.62/15]
Metrics/MethodLength: Method has too many lines. [11/10]
app/models/user.rb
Outdated
| :recoverable, :rememberable, :trackable, :validatable, :omniauthable, omniauth_providers: [:google_oauth2] | ||
|
|
||
| validates :email, uniqueness: true | ||
| has_many :subjects |
There was a problem hiding this comment.
Rails/HasManyOrHasOneDependent: Specify a :dependent option.
| # arbitrary gems may also be filtered via: | ||
| # config.filter_gems_from_backtrace("gem name") | ||
|
|
||
| Shoulda::Matchers.configure do |config| |
There was a problem hiding this comment.
Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - config.
|
|
||
| context 'when did not found unmatching latter emails' do | ||
| it 'return only t matches' do | ||
| described_class.find_matching_letter('t').each { |user| expect(user.email).not_to include(petras.email, povilas.email) } |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [128/120]
| # | ||
| # It's strongly recommended that you check this file into your version control system. | ||
|
|
||
| ActiveRecord::Schema.define(version: 20_180_405_132_238) do |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [26/25]
| # frozen_string_literal: true | ||
|
|
||
| class DeviseCreateUsers < ActiveRecord::Migration[5.1] | ||
| def change |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [15/10]
| # Add a new OmniAuth provider. Check the wiki for more information on setting | ||
| # up on your models and hooks. | ||
| # config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo' | ||
| config.omniauth :google_oauth2, '398135549725-pm9shjiu4fta240nirdbb8baihakf4ph.apps.googleusercontent.com', 'yOBwbclBDVDdB_hsr7PCezIH' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [136/120]
| config.stretches = Rails.env.test? ? 1 : 11 | ||
|
|
||
| # Set up a pepper to generate the hashed password. | ||
| # config.pepper = '9a0cd2508e19f24dcf762310977ed5b530c7e967d9f92c4011ddba30047ad115c5946236098878e9df43fded3ef3f027fb1e71b9954847666b0c24a722c77f60' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [150/120]
| # confirmation, reset password and unlock tokens in the database. | ||
| # Devise will use the `secret_key_base` as its `secret_key` | ||
| # by default. You can change it below and use your own secret key. | ||
| # config.secret_key = 'f7ffe60b93af3e9eb4820b35e765b23edd5e61d53cf8c465737e0b1886c01b21748653f27cad511b9c0dd636a70336574f67a765fa22a35a9ef10dffc3b59248' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [154/120]
| @@ -0,0 +1,8 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class Subject < ApplicationRecord | |||
There was a problem hiding this comment.
Style/Documentation: Missing top-level class documentation comment.
app/models/user.rb
Outdated
| validates :email, uniqueness: true | ||
| has_many :subjects | ||
|
|
||
| def self.from_omniauth(auth) |
There was a problem hiding this comment.
Metrics/AbcSize: Assignment Branch Condition size for from_omniauth is too high. [20.62/15]
Metrics/MethodLength: Method has too many lines. [11/10]
app/models/user.rb
Outdated
| :recoverable, :rememberable, :trackable, :validatable, :omniauthable, omniauth_providers: [:google_oauth2] | ||
|
|
||
| validates :email, uniqueness: true | ||
| has_many :subjects |
There was a problem hiding this comment.
Rails/HasManyOrHasOneDependent: Specify a :dependent option.
| # arbitrary gems may also be filtered via: | ||
| # config.filter_gems_from_backtrace("gem name") | ||
|
|
||
| Shoulda::Matchers.configure do |config| |
There was a problem hiding this comment.
Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - config.
|
|
||
| context 'when did not found unmatching latter emails' do | ||
| it 'return only t matches' do | ||
| described_class.find_matching_letter('t').each { |user| expect(user.email).not_to include(petras.email, povilas.email) } |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [128/120]
| # | ||
| # It's strongly recommended that you check this file into your version control system. | ||
|
|
||
| ActiveRecord::Schema.define(version: 20_180_405_132_238) do |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [26/25]
| # frozen_string_literal: true | ||
|
|
||
| class DeviseCreateUsers < ActiveRecord::Migration[5.1] | ||
| def change |
There was a problem hiding this comment.
Metrics/MethodLength: Method has too many lines. [15/10]
| # Add a new OmniAuth provider. Check the wiki for more information on setting | ||
| # up on your models and hooks. | ||
| # config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo' | ||
| config.omniauth :google_oauth2, '398135549725-pm9shjiu4fta240nirdbb8baihakf4ph.apps.googleusercontent.com', 'yOBwbclBDVDdB_hsr7PCezIH' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [136/120]
| config.stretches = Rails.env.test? ? 1 : 11 | ||
|
|
||
| # Set up a pepper to generate the hashed password. | ||
| # config.pepper = '9a0cd2508e19f24dcf762310977ed5b530c7e967d9f92c4011ddba30047ad115c5946236098878e9df43fded3ef3f027fb1e71b9954847666b0c24a722c77f60' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [150/120]
| # confirmation, reset password and unlock tokens in the database. | ||
| # Devise will use the `secret_key_base` as its `secret_key` | ||
| # by default. You can change it below and use your own secret key. | ||
| # config.secret_key = 'f7ffe60b93af3e9eb4820b35e765b23edd5e61d53cf8c465737e0b1886c01b21748653f27cad511b9c0dd636a70336574f67a765fa22a35a9ef10dffc3b59248' |
There was a problem hiding this comment.
Metrics/LineLength: Line is too long. [154/120]
| @@ -0,0 +1,64 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| class SubjectsController < ApplicationController | |||
There was a problem hiding this comment.
Style/Documentation: Missing top-level class documentation comment.
| def after_sign_in_path_for(user) | ||
| # binding.pry | ||
| subjects_url(user) | ||
| end |
There was a problem hiding this comment.
Layout/DefEndAlignment: end at 10, 1 is not aligned with def at 7, 2.
| @@ -0,0 +1,6 @@ | |||
| body.controller-devise { | |||
| background-color: #f2f7ff; | |||
| background-image: url('./backgroundimage.jpeg'); | |||
| @@ -0,0 +1,6 @@ | |||
| body.controller-devise { | |||
| background-color: #f2f7ff; | |||
There was a problem hiding this comment.
Color literals like #f2f7ff should only be used in variable declarations; they should be referred to via variable everywhere else.
| @@ -0,0 +1,6 @@ | |||
| body.controller-devise { | |||
There was a problem hiding this comment.
Avoid qualifying class selectors with an element.
#5
Finished user authentication, the user now can log in (authenticate) in the following ways:
** Using his Gmail account (gem => omniauth)
** Using regular registration form with email and password (gem => devise)
Only authenticated users can access subjects: view, edit, create, delete.
Subject content can be written in rich format (gem => trix)
Only subject author (creator), can delete, update post, for other people it is only accessible for a view
Added rspec user tests