decidim-census: authorise via census using a CSV file (engine) #15

danigb · 2017-11-14T10:49:10Z

Fixes #7

- Refactor CensusCsvService into CsvData model - Add a background job to remove duplicates - It only displays the number of unique census rows

…for the current organization - Move AuthorizeWithAge to lib/extensions

xredo · 2017-11-20T11:46:51Z

db/schema.rb


  # These are extensions that must be enabled in order to support this database
  enable_extension "plpgsql"

+  create_table "censuses", force: :cascade do |t|


@danigb Watch out this. You shouldn't have the decidim_censuses_censuses and the censuses table. I imagine that this censuses table is produced by some legacy code.

xredo · 2017-11-20T11:47:41Z

db/schema.rb

@@ -161,6 +174,23 @@
    t.index ["decidim_root_commentable_type", "decidim_root_commentable_id"], name: "decidim_comments_comment_root_commentable"
  end

+  create_table "decidim_dummy_resources", force: :cascade do |t|


I don't think that this should be into your database. Is it required?

xredo · 2017-11-20T11:49:22Z

decidim-censuses/README.md

+
+> Add census authorization to Decidim platform
+
+Allows to upload a census CSV file to perform authorizations agains real user age.


I would rewrite this as:

Allows to upload a census CSV file to perform authorizations against real users parameterised by their age.

xredo · 2017-11-20T11:49:44Z

decidim-censuses/README.md

+Add this line to your application's Gemfile:
+
+```ruby
+gem 'decidim-census'


It's decidim-censuses

Impresionante, no se te escapa una 👏

xredo · 2017-11-20T11:50:27Z

decidim-censuses/README.md

+Create a dummy app:
+
+```bash
+$ bin/rails decidim:generate_test_app


If we are giving instructions to the user to generate a test app, maybe we don't need to check it out into the source code. What do you think?

The key here is that the README was written with the idea of publish the engine independently from the app.

I've rewritten this message as follows: "Create a dummy app in your application (if not present):"

xredo · 2017-11-20T12:05:51Z

decidim-censuses/app/controllers/decidim/censuses/admin/census_uploads_controller.rb

+          authorize! :destroy, Census
+          Census.delete_all
+          flash[:notice] = t('.success')
+          redirect_to census_uploads_path


you can use: redirect_to census_uploads_path, notice: t('.success') which is shorter.

xredo · 2017-11-20T12:08:36Z

decidim-censuses/app/jobs/decidim/censuses/remove_duplicates_job.rb

+                           .having('count(id_document)>1')
+                           .map(&:id_document)
+
+        duplicated.each do |id_doc|


Instead of each, prefer find_each. Find each is less performant for small groups of objects but is needed if you can retrieve many objects.

xredo · 2017-11-20T12:10:04Z

decidim-censuses/app/jobs/decidim/censuses/remove_duplicates_job.rb

+      queue_as :default
+
+      def perform
+        duplicated = Census.select(:id_document)


Every time you are assigning a local variable, think if it's worth it to extract this to a method. It usually improves readability.

def perform duplicated_census.each do |id_doc| end end private def duplicated_census Census.select(:id_document).blablabla end

xredo · 2017-11-20T12:17:43Z

db/migrate/20171110120821_create_decidim_census_censuses.decidim_censuses.rb

+  def change
+    create_table :decidim_censuses_censuses do |t|
+      t.string :id_document
+      t.string :birthdate


I have a question. Why we save the birthdate as a string? I see more advantages of using a date object.

Agree. It departures from the standard, and thats not a good thing. Also it's not clear if it's really a date, and it's not documented the format. So, rolling back to a date field!

xredo · 2017-11-20T12:18:22Z

decidim-censuses/app/mailers/decidim/censuses/application_mailer.rb

@@ -0,0 +1,10 @@
+module Decidim


If we are not going to send emails, I would remove this class (of everything keeps working fine, of course 😅)

xredo

I have added a couple of additional comments worth checking out.

xredo · 2017-11-21T07:59:13Z

decidim-censuses/lib/decidim/censuses/extensions/authorize_with_age.rb

+          @birthdate ||= Date.strptime(authorization.metadata['birthdate'], '%Y/%m/%d')
+        end
+
+        def current_age


I feel that this implementation is too complex.

I'm very bad at understanding low level implementations, so, when I'm starting to reach this kind of solutions I tend to search for simpler solutions. The one that comes to my mind is:

return status(:invalid, fields: [:birthdate]) unless minimum_age_check? def minimum_age_check? (birthdate + minimum_age.years) < Date.current end

What do you think?

Desde luego es simple y elegante 👏

xredo · 2017-11-21T08:08:15Z

decidim-censuses/lib/decidim/censuses/extensions/authorize_with_age.rb

+          now.year - birthdate.year - (extra_year ? 0 : 1)
+        end
+
+        def minimum_age


When trying to convert something to an integer in Ruby it's a common practice using the .to_i object method. Every object implements it (even nil!)

It returns the integer for integers or 0 if it's not present. So, this could be an alternative implementation:

def minimum_age return DEFAULT_MINIMUM_AGE if permission_options['edad'].to_i.zero? permission_options['edad'].to_i end

However, this also has drawbacks. The number 0 does not represent a blank value, and in order to understand this piece of code you need to understand the internals of Ruby.

It's worth thinking about it. What implementation do you like more?

I've decided to follow this way after reading at https://stackoverflow.com/questions/49274/safe-integer-parsing-in-ruby:

you might want to watch for strings that are valid non-decimal numbers, such as those starting with 0x for hex and 0b for binary, and potentially more tricky numbers starting with zero that will be parsed as octal.

Probably too much precaution. I prefer more idiomatic ways, but I'm not 100% convinced about your solution (mostly because of two calls to to_i method, but probably ruby is now smart enough)...

And by the way: :hello.to_i fails ;-)

xredo · 2017-11-21T08:10:05Z

@danigb I'm reviewing the feature, and I have found the following.

Census needs to be associated with an organization.

We need to add a belongs_to association with an organization. And every action performed against the Census needs to be scoped by this organization:

ej:

Census.where(organization: current_organization).delete_all

This way we can treat censuses independently for every organization.

- The authorization form requires a date, and the authorization handler verifies the date correspond to the census in order to validate - The date is now stored in the database as a date - Remove unused tables from schema.db - Improve engine's README - Minor improvements and fixes

- delete_all is renamed to destroy

- Rename `to_id_document` into `normalize_id_document` - Rename `to_birthdate` into `parse_date`

xredo · 2017-11-21T13:00:47Z

decidim-census/lib/decidim/census/extensions/authorize_with_age.rb

@@ -0,0 +1,46 @@
+require 'pry'


We need to remove this. It's only for debug purposes.

xredo · 2017-11-21T13:01:05Z

decidim-census/app/services/census_authorization_handler.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'pry'


We need to remove this. It's only for debug purposes

xredo · 2017-11-21T13:01:27Z

decidim-census/spec/services/census_authorization_handler_spec.rb

@@ -0,0 +1,39 @@
+require 'spec_helper'
+require 'pry'


We need to remove this. Only for debug purposes.

xredo · 2017-11-21T13:06:56Z

decidim-census/app/models/decidim/census/csv_data.rb

+        @errors = []
+        @values = []
+
+        CSV.foreach(@file, headers: true, col_sep: ';') do |row|


For me it's surprising that when I am instantiating an object it performs some kind of action. This is not a mistake. It's just because I am too used to using the form:

CsvData.new(file).call

However, I imagine that your point is that the object processing should only be run once (with the same file param). So there is no need to have a method call that can be called several times.

It's just a reflexion. I'm not expecting any change here.

Yes... Probably CsvData.new.call(file) is better...

Now I'm looking into this issue:

For me it's surprising that when I am instantiating an object it performs some kind of action.

Yes, I understand... but you only know that if you look at the code inside the class ;-) In fact, you can treat it like a kind of object initialization...

I imagine that your point is that the object processing should only be run once

Yes. Exactly.

Probably CsvData.new(file).call it's more idiomatic, but I'm afraid to have an object instance in an inconsistency state... What if you forget to call call?

xredo · 2017-11-21T13:16:44Z

decidim-census/app/controllers/decidim/census/admin/censuses_controller.rb

+
+        def create
+          authorize! :create, CensusDatum
+          if params[:file]


This implementation is correct. However, I would have used a more Rails like approach to the problem.

def create authorize! :create, CensusDatum csv_data = CsvData.new(file: params[:file]) if csv_file.valid? csv_data.parse CensusDatum.insert_all(current_organization, csv_data.values) RemoveDuplicatesJob.perform_later(current_organization) flash[:notice] = t('.success', count: data.values.count, errors: data.errors.count) end redirect_to censuses_path end

And then, In CsvData:

class CsvData include ActiveModel::Model attr_accessor :file validates :file, presence: true end

However, I don't believe that we should change the current code, because adding ActiveModel here is overkill for what we need. I'm making this comment as an example of alternate implementations.

Great! I'll think about it

danigb added 30 commits November 9, 2017 13:20

Create engine via 'rails plugin new'

edf57b3

Setup Decidim::Census::AdminEngine

084ace3

Rubocop fixings

3cccbe3

Setup admin views

9c64ea9

Rename engine from census to decidim-census

bfbf2b1

Remove tests. Add Census model

b65bb82

Census model and controller, with authorization

f500ea6

Rename to decidim-censuses

ded70a4

Fix namespace

3e307fd

Trying to run test without success

4a291f4

Add CensusAuthorizationHandler

333b6cd

flash

69bf917

prepend a module to Decidim::ActionAuthorizer

01f26d8

Add metadata to the authorization objectç

bc09bcd

Perform the actual validation against the user's census age

d756d25

Generate decidim dummy app

1c7d501

Fix model specs

ef336ce

Test CensusAuthorizationHandler

5d578a7

Test AuthorizeWithAge

c3fabaa

Generate random data to test.

a6169dc

Import CSV using activerecord-import (with NO time improvement)

de61a28

Use SQL to insert the rows

a71ab33

Use factories from decidim and Walden helpers

f63e165

Merge: insert and remove duplicates. Add id_document normalization

a7e12b6

Authorization Handler normalizes id document

a1315f7

Fixes (almost) the engine tests

fe74409

Refactor and background jobs

94eb412

- Refactor CensusCsvService into CsvData model - Add a background job to remove duplicates - It only displays the number of unique census rows

- Add setup instructions if CensusAuthorizationHandler is not active …

effe1d1

…for the current organization - Move AuthorizeWithAge to lib/extensions

Add a button to remove all census data

e362c75

Add test for destroy_all action

62ab098

danigb added 5 commits November 17, 2017 10:34

Refactor functionallity into Censuses::Status

1067b62

Add locales for ca, es and en

3231f01

Fix tests! Yeah....

3b7aa13

Little refactor to clean a little bit CensusUploadsController

b4c0e39

Fix routes: census admin engine inside '/admin'

29d5e6b

danigb changed the title ~~decidim-census Engine WIP~~ decidim-census: authorise via census using a CSV file (engine) Nov 20, 2017

xredo suggested changes Nov 20, 2017

View reviewed changes

xredo suggested changes Nov 21, 2017

View reviewed changes

xredo and others added 11 commits November 21, 2017 09:20

Update locales

8931cb1

Remove unused code

1f14c39

Rename decidim-censuses to decidim-census

b7fe0a4

Merge branch 'engine' of github.com:MarsBased/decidim-diba into engine

b07e672

CensusDatum is scoped by organization

4930e99

Rename CensusUploadsController to CensusesController

470af69

CensusesController now conforms REST convention

2c0c9b9

- delete_all is renamed to destroy

Simplify authorization domain logic

ae78928

Some minor refactorings:

f177481

- Rename `to_id_document` into `normalize_id_document` - Rename `to_birthdate` into `parse_date`

Update texts

6434b4a

xredo reviewed Nov 21, 2017

View reviewed changes

xredo added 3 commits November 21, 2017 14:28

More text changes

1b39e14

Remove pry requires

6150082

Small text & usability fixes

8abe20b

xredo merged commit 70b6b56 into DiputacioBarcelona:master Nov 21, 2017

xredo deleted the engine branch November 21, 2017 14:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

decidim-census: authorise via census using a CSV file (engine) #15

decidim-census: authorise via census using a CSV file (engine) #15

danigb commented Nov 14, 2017 •

edited

Loading

xredo Nov 20, 2017

xredo Nov 20, 2017

xredo Nov 20, 2017

xredo Nov 20, 2017

danigb Nov 20, 2017

xredo Nov 20, 2017

danigb Nov 20, 2017

xredo Nov 20, 2017

xredo Nov 20, 2017

xredo Nov 20, 2017

xredo Nov 20, 2017

danigb Nov 20, 2017

xredo Nov 20, 2017

xredo left a comment

xredo Nov 21, 2017

danigb Nov 21, 2017

xredo Nov 21, 2017

danigb Nov 21, 2017

xredo commented Nov 21, 2017

xredo Nov 21, 2017

xredo Nov 21, 2017

xredo Nov 21, 2017

xredo Nov 21, 2017

danigb Nov 22, 2017

danigb Nov 22, 2017

xredo Nov 21, 2017

danigb Nov 22, 2017


		> Add census authorization to Decidim platform

		Allows to upload a census CSV file to perform authorizations agains real user age.

decidim-census: authorise via census using a CSV file (engine) #15

decidim-census: authorise via census using a CSV file (engine) #15

Conversation

danigb commented Nov 14, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xredo left a 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

xredo commented Nov 21, 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

danigb commented Nov 14, 2017 •

edited

Loading