Url validation and Authorisation #48

Nimmyjv · 2017-04-07T05:58:37Z

No description provided.

redtachyons · 2017-04-07T06:06:41Z

app/policies/collection_policy.rb

+     def destroy?
+      record.user == user
+     end
+end


Indentation

redtachyons · 2017-04-07T06:07:03Z

config/locales/en.yml

@@ -21,3 +21,4 @@

 en:
  hello: "Hello world"
+


Rubocop warns about not having newline at end of file. Either way, the last line shouldn't have the indentation.

nithinbekal · 2017-04-07T06:25:44Z

app/services/collections/delete.rb

@@ -19,6 +19,7 @@ def call
    attr_reader :user, :collection

    def delete_collection
+      Pundit.authorize(user, collection, :destroy?)


This should be done in the controller, not here.

nithinbekal · 2017-04-07T06:26:02Z

app/services/websites/create.rb

@@ -1,3 +1,4 @@
+# frozen_string_literal: true


Not needed. Ignore this warning if rubocop shows it.

nithinbekal · 2017-04-07T06:26:21Z

app/validators/url_validator.rb

+    false
+  end
+
+  def valid_host? host


Parentheses around args.

Afaik rubocop suggests to not use parenthesis if the number of arguments are 1

I doubt that. Rubocop follows bbatsov's styleguide, which prefers parentheses all the time, and I don't remember seeing a warning about this even though I always use parens around single args.

Maybe you're thinking about the case where there are no args?

nithinbekal · 2017-04-07T06:26:28Z

app/validators/url_validator.rb

+    host.present? && valid_characters?(host)
+  end
+
+  def valid_characters? host


Parentheses around args.

nithinbekal · 2017-04-07T06:28:31Z

app/models/website.rb

 class Website < ApplicationRecord
+  before_validation :add_protocol_to_website 
+  validates :url, url: true


Group different types of statements (callbacks, validations, associations, etc) together, and separate them by empty lines.

nithinbekal · 2017-04-07T06:53:18Z

@Nimmyjv When you submit a PR that fixes a specific issue, please add "fixes #issueNo" in the description. It will be automatically connected to it.

nithinbekal · 2017-04-07T09:22:52Z

app/models/website.rb

@@ -38,6 +40,6 @@ def fetch_meta_description
  private

  def add_protocol_to_website
-    self.url = (self.url[/^http:\/\//] || self.url[/^https:\/\//]) ? self.url : "http://#{self.url}"
+    self.url = url[/^http:\/\//] || url[/^https:\/\//] ? url : "http://#{url}"


This is difficult to read. In Leo, we rewrote it like this:

def add_protocol_to_url return if protocol_present? self.url = "https://#{url}" end def protocol_present? return false if url.blank? url[/^http:\/\//] || url[/^https:\/\//] end

nithinbekal · 2017-04-07T09:23:58Z

app/validators/url_validator.rb

@@ -1,3 +1,4 @@
+


Blank line not needed.

nithinbekal · 2017-04-07T09:24:16Z

app/policies/collection_policy.rb

@@ -1,6 +1,6 @@
+# frozen_string_literal: true


As mentioned before, remove this and ignore frozen_string_literal warnings from rubocop. Also, edit rubocop.yml to ignore these warnings.

nithinbekal · 2017-04-07T09:24:37Z

app/controllers/application_controller.rb

@@ -1,3 +1,4 @@
+# frozen_string_literal: true


Remove this.

nithinbekal · 2017-04-07T11:20:37Z

app/controllers/collections_controller.rb

@@ -7,7 +7,9 @@ def create
  end

  def destroy
-    Collections::Delete.call(params[:id], current_user)
+    collection = Collection.find(params[:id])
+    Pundit.authorize(current_user, collection, :destroy?)


Is Pundit. required? authorize(...) works, I believe.

nithinbekal · 2017-04-07T11:21:56Z

app/services/collections/delete.rb

@@ -19,7 +19,7 @@ def call
    attr_reader :user, :collection

    def delete_collection
-      CollectionWebsite.where('collection_id = ?', collection.id).destroy_all
+      CollectionWebsite.where(collection_id: collection.id).destroy_all


Can't this be achieved by doing dependent: destroy on has_many :collection_websites?

nithinbekal · 2017-04-07T16:41:31Z

LGTM. 👍

redtachyons · 2017-04-10T04:35:00Z

app/controllers/application_controller.rb

+
+  def user_not_authorized
+    flash[:alert] = 'You are not authorized to perform this action.'
+    redirect_to(request.referer || root_path)


You can write this like redirect_back(fallback_location: root_path)

…edpanthers.co is added it is stored as http://redpanthers.co When again redpanthers.co added to the same list, it was added because it is compared with http://redpanthers.co To prevent this, when url is added itself, it is checked whether starts with http or https. Else, http is added and then compared

…ethod has been added called redirect_back.

nithinbekal · 2017-04-10T13:20:29Z

app/assets/javascripts/custom.js

@@ -1,4 +1,4 @@
-$(document).ready(function() {
+$(document).ready(function() { 


Trailing space.

nithinbekal · 2017-04-10T13:21:03Z

app/models/invite.rb

@@ -1,5 +1,4 @@
 class Invite < ApplicationRecord
  has_secure_token
-


Leave a blank line between different kind of declarations.

nithinbekal · 2017-04-10T13:24:16Z

@Nimmyjv Are the tests running fine? I think these changes might be breaking some of the tests, or causing HTTP requests. If there are HTTP requests to 3rd parties, use webmock to stub out the requests.

nithinbekal · 2017-04-11T13:28:49Z

app/models/invite.rb

@@ -1,5 +1,5 @@
 class Invite < ApplicationRecord
  has_secure_token
-
+  


Trailing whitespace.

nithinbekal · 2017-04-11T13:41:30Z

app/views/static_pages/_collection.html.erb

Trailing whitespace, and unnecessary 2nd blank line.

nithinbekal · 2017-04-11T13:43:32Z

app/services/websites/create.rb

+              url
+            else
+              "http://#{url}"
+            end


Extract the condition out, so you can rewrite it like this:

url = "http://#{url}" unless protocol_given?(url)

nithinbekal · 2017-04-11T13:45:29Z

db/seeds.rb

@@ -20,4 +25,4 @@
  end
 end

-AdminUser.create!(email: '[email protected]', password: 'password', password_confirmation: 'password')
+AdminUser.first_or_create!(email: '[email protected]', password: 'password', password_confirmation: 'password')


Do we really need first_or_create in seeds.rb? It is only run in an empty database, right?

nithinbekal · 2017-04-11T13:46:43Z

db/seeds.rb


-urls.each do |url|
-  user.websites.create!(url: url)
+user = User.first_or_create!(name: 'abc', email: '[email protected]', password: '123456',                    password_confirmation: '123456')


Whitespace in the middle of line.

nithinbekal · 2017-04-11T13:48:37Z

db/seeds.rb

+  collection = user.collections.create!(name: name)
+  urls.each do |url|
+    collection.websites.create!(url: url)
+  end
 end


In seeds.rb, you don't really need all this complexity, since you know what data you want to add. You can write it like this:

user = User.create!(name: 'abc', email: '[email protected]', password: '123456', password_confirmation: '123456') collection = user.collections.create!(name: 'First Collection') collection.websites.create!(url: 'https://redpanthers.co')

When adding seed data, it would be good to add valid url, since you will be using it in development.

nithinbekal · 2017-04-11T13:50:11Z

test/services/websites/create_test.rb

+      Websites::Create.call(params, user)
+
+      user.reload
+      assert_equal 1, user.site_number, 'site number'


Does this field contain number of websites a user has? Then can we call it websites_count instead? This seems an awkward name for the field.

(Do this in a separate PR rather than mixing these.)

nithinbekal · 2017-04-12T12:11:46Z

app/services/websites/create.rb

+
+    def protocol_given?(url)
+      url if (url.start_with? 'http://') || (url.start_with? 'https://')
+    end


Don't return url. A method ending with ? should return true or false.

nithinbekal · 2017-04-12T12:12:24Z

app/views/static_pages/_collection.html.erb

+            <% end %>
+          <% end %>
+          <% if collection_nil.empty? %>
+            <% @collection_names.any? %>


What does this do? It just seems to return true or false and render nothing.

nithinbekal · 2017-04-12T12:14:12Z

test/services/websites/create_test.rb

@@ -11,7 +11,7 @@ class CreateTest < ActiveSupport::TestCase
      Websites::Create.call(params, user)

      user.reload
-      assert_equal 1, user.site_number, 'site number'
+      assert_equal 1, user.websites.count, 'websites_count'


I meant renaming site_number to websites_count since that is more accurately describing what it contains, not user.websites.count.

NimmyVipin added 2 commits April 7, 2017 11:15

Add validators folder, url validation

f04521a

Authorize collection added by each user with pundit gem

16e87d1

redtachyons reviewed Apr 7, 2017

View reviewed changes

app/policies/collection_policy.rb Outdated

def destroy?

record.user == user

end

end

Copy link

redtachyons Apr 7, 2017

Choose a reason for hiding this comment

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

Indentation

redtachyons reviewed Apr 7, 2017

View reviewed changes

nithinbekal suggested changes Apr 7, 2017

View reviewed changes

NimmyVipin added 2 commits April 7, 2017 12:24

Modify as run pronto

55b69bb

Authorization moved from service to controller

95b2f2d

nithinbekal suggested changes Apr 7, 2017

View reviewed changes

NimmyVipin added 4 commits April 7, 2017 16:02

Edit rubocop.yml to avoid frozenstringliteralwarnings

2a7915c

Modify method to add protocol to url

494e5e8

Fix rubocop warnings

abf759e

Correct method name

dde10d5

nithinbekal reviewed Apr 7, 2017

View reviewed changes

NimmyVipin added 3 commits April 7, 2017 17:55

Remove query to delete from join table

f32a6b6

Modify authorize method

8695d57

change https to http

b074c36

redtachyons reviewed Apr 10, 2017

View reviewed changes

NimmyVipin added 4 commits April 10, 2017 16:10

Destroy collectionWebsites also when collection deleted

1c1ee84

In Rails 5, redirect_to :back has been deprecated and instead a new m…

93f18c8

…ethod has been added called redirect_back.

Add tool tips in multiple list page

cc68662

nithinbekal reviewed Apr 10, 2017

View reviewed changes

NimmyVipin added 3 commits April 11, 2017 18:24

Edit rubocop file to ignore regex literal

917f122

change redirection path

f370efd

Spacing correction

1fa8094

NimmyVipin added 2 commits April 11, 2017 18:30

Solve error while run rake db:seed

2528f30

Modify test file to fix failures

9516584

nithinbekal suggested changes Apr 11, 2017

View reviewed changes

NimmyVipin added 4 commits April 12, 2017 16:57

Run pronto

89dff22

Code refactored

0dbd52d

Indentation corrected

ed02b10

change name

19a9a7d

nithinbekal suggested changes Apr 12, 2017

View reviewed changes

remove return value

1a53955

		@@ -1,4 +1,4 @@
		$(document).ready(function() {
		$(document).ready(function() {

		@@ -1,5 +1,4 @@
		class Invite < ApplicationRecord
		has_secure_token

		@@ -1,5 +1,5 @@
		class Invite < ApplicationRecord
		has_secure_token

Url validation and Authorisation #48

Are you sure you want to change the base?

Url validation and Authorisation #48

Conversation

Nimmyjv commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithinbekal commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithinbekal commented Apr 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithinbekal commented Apr 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment