bio field now part of User; status moved off of DrupalUser #1497

jywarren · 2017-07-04T17:13:02Z

This also establishes a 'token' field for API use and deprecates the location_privacy field of User.

all tests pass -- rake test:all
code is in uniquely-named feature branch, and has been rebased on top of latest master (especially if you've been asked to make additional changes)
pull request is descriptively named with #number reference back to original issue

@ryzokuken - here I'm initializing a token field. You can autogen random tokens in this PR if you like; add to the migration file in each user. We only have to generate them for users of non-zero status.

jywarren · 2017-07-04T17:25:03Z

db/migrate/20170704165711_add_user_bio_and_token.rb

+    # copy bios into new fields for non-spam users
+    DrupalUsers.where('status != 0').each do |u|
+      user = u.user
+      user.status = u.status


Right here you could do something like user.token = SecureRandom.hex

@jywarren okay, I am setting up plots2 again and will do this hopefully by today.

@jywarren I don't think SecureRandom.hex will ensure uniqueness, though. What do you say?

We need to regenerate the schema file. Doing this now!

@jywarren great.

jywarren · 2017-07-04T19:50:24Z

You can concatenate the UID to the beginning?

…

On Jul 4, 2017 3:48 PM, "Ujjwal Sharma" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In db/migrate/20170704165711_add_user_bio_and_token.rb <#1497 (comment)>: > @@ -0,0 +1,23 @@ +class AddUserBioAndToken < ActiveRecord::Migration + def up + add_column :rusers, :bio, :text + add_column :rusers, :token, :string + add_column :rusers, :status, :integer, default: 0 + remove_column :rusers, :location_privacy + + # copy bios into new fields for non-spam users + DrupalUsers.where('status != 0').each do |u| + user = u.user + user.status = u.status @jywarren <https://github.com/jywarren> I don't think SecureRandom.hex will ensure uniqueness, though. What do you say? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ-SA2Kg-tK1gwGwWy4EOAOnt661Tks5sKpbsgaJpZM4ONkX1> .

ryzokuken · 2017-07-04T19:54:25Z

@jywarren SecureRandom.uuid sounds better. Although I would like to point out:

This is because the probability that the 16-byte or 128-bit token is nonunique is so vanishingly small that it is virtually zero. There is only a 50% chance of there being any repetitions after approximately 2^64 = 18,446,744,073,709,551,616 = 1.845 x 10^19 tokens have been generated. If you start generating one billion tokens per second, it will take approximately 2^64/(109360024*365.25) = 600 years until there is a 50% chance of there having occurred any repetitions at all.

jywarren · 2017-07-04T20:00:14Z

That's fine. We need to regenerate the schema too, and perhaps remove a few unused tables like searches, location tags

ryzokuken · 2017-07-04T20:00:42Z

Basically, these are our options:

SecureRandom.uuid           # => "8efb5d40-32d8-43c8-a92d-d5f048c8729c"
SecureRandom.hex            # => "bad6650fc0142451e624bd89b6aa3acf"
SecureRandom.urlsafe_base64 # => "vp6xjBgi48Jgag6dqH8niw"

The last one looks rather short, doesn't it?

P.S: All of these have the exact same entropy.

jywarren · 2017-07-04T20:06:56Z

I trust your judgement on this!

…

On Jul 4, 2017 4:00 PM, "Ujjwal Sharma" ***@***.***> wrote: Basically, these are our options: SecureRandom.uuid # => "8efb5d40-32d8-43c8-a92d-d5f048c8729c"SecureRandom.hex # => "bad6650fc0142451e624bd89b6aa3acf"SecureRandom.urlsafe_base64 # => "vp6xjBgi48Jgag6dqH8niw" — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ3UwAHUsDZmKZpIi7y6WlqJblKtTks5sKpnqgaJpZM4ONkX1> .

ryzokuken · 2017-07-04T20:12:30Z

@jywarren SecureRandom.uuid looks a little more verbose, but I will use it. Will make the required changes as soon as the environment is setup locally. A few thing I would like to confirm though:

How do I add to this PR? Do I copy your branch, make changes and make a PR for your fork's branch? When you push my changes, they will show up in here, I guess.
Apart from adding user.token = SecureRandom.uuid in https://github.com/publiclab/plots2/pull/1497/files/4014dfc777c83f9f97491676b9ffd0aadf2250c4#diff-85d8a967f5e9f12cfb43f842e560d394, what all other tasks do I need to perform? Which specific tables need to be removed?

jywarren · 2017-07-04T20:17:42Z

I can remove the tables, that's not a big deal, or if you like you can drop: `searches` and `location_tags`. You should be able to open a PR against the feature branch I'm using, and I can merge it into my feature branch :-) then the PRs combine!

…

On Jul 4, 2017 4:12 PM, "Ujjwal Sharma" ***@***.***> wrote: @jywarren <https://github.com/jywarren> SecureRandom.uuid looks a little more verbose, but I will use it. Will make the required changes as soon as the environment is setup locally. A few thing I would like to confirm though: 1. How do I add to this PR? Do I copy your branch, make changes and make a PR for your fork's branch? When you push my changes, they will show up in here, I guess. 2. Apart from adding user.token = SecureRandom.uuid in https://github.com/publiclab/plots2/pull/1497/files/ 4014dfc#diff- 85d8a967f5e9f12cfb43f842e560d394 <https://github.com/publiclab/plots2/pull/1497/files/4014dfc777c83f9f97491676b9ffd0aadf2250c4#diff-85d8a967f5e9f12cfb43f842e560d394>, what all other tasks do I need to perform? Which specific tables need to be removed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ3HCSFWZWzpjHYay22Xz8XJCuFHGks5sKpyugaJpZM4ONkX1> .

ryzokuken · 2017-07-04T20:21:41Z

@jywarren Awesome. I will look more into safely dropping tables from the schema while bundle install executes. On another note, are we using any dependencies we could avoid? The CI build takes way too much time to finish.

jywarren · 2017-07-04T20:27:21Z

You can drop tables in the migration itself and then copy your new generated local schema over the schema example file. @icarito was talking about caching some parts to save time. I also think we could move some of our tests from integration into lower level unit tests (and likewise actual code) as unit tests are more self contained and less context dependent, and so run much faster. You could open an issue around optimizing tests!

…

On Jul 4, 2017 4:21 PM, "Ujjwal Sharma" ***@***.***> wrote: @jywarren <https://github.com/jywarren> Awesome. I will look more into safely dropping tables from the schema while bundle install executes. On another note, are we using any dependencies we could avoid? The CI build takes way too much time to finish. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ7MTrd2EO3Ul0ImhXEW-Oj_0LBtJks5sKp7VgaJpZM4ONkX1> .

ryzokuken · 2017-07-04T20:30:23Z

Definitely. One of the new contributors was also talking about Makefiles which reminded of the PR I once made for the same. I will consult @icarito and make one for real this time.

ryzokuken · 2017-07-04T20:43:49Z

@jywarren we figured out a workaround for Gem::LoadError: mysql2 is not part of the bundle. Add it to your Gemfile., right? I am getting this error :/

jywarren · 2017-07-05T18:21:22Z

I think you figured this out? Sorry i wasn't able to respond faster!

ryzokuken · 2017-07-05T18:25:07Z

@jywarren Yes, you already fixed it, I was using an older version :P

I will submit a PR for this one in a moment, thanks for your patience.

ryzokuken · 2017-07-05T22:15:11Z

@jywarren Made a PR. Please look into it. Awaiting your approval before going ahead with the migration.

ryzokuken · 2017-07-06T17:28:11Z

@jywarren

ActiveRecord::StatementInvalid: Mysql2::Error: BLOB/TEXT column 'params' used in key specification without a key length: CREATE  INDEX `poly_params_request_index` ON `impressions` (`impressionable_type`, `impressionable_id`, `params`)

/app/db/schema.rb:193:in `block in <top (required)>'

/app/db/schema.rb:14:in `<top (required)>'

Mysql2::Error: BLOB/TEXT column 'params' used in key specification without a key length

/app/db/schema.rb:193:in `block in <top (required)>'

/app/db/schema.rb:14:in `<top (required)>'

Tasks: TOP => db:schema:load

ryzokuken · 2017-07-06T19:22:38Z

@jywarren There's a pending migration. Do you want me to make a PR with the migration done and schema replaced?

jywarren · 2017-07-06T19:25:55Z

Sure, or I can do it in an hour. Just forgot a step.

…

On Jul 6, 2017 3:22 PM, "Ujjwal Sharma" ***@***.***> wrote: @jywarren <https://github.com/jywarren> There's a pending migration. Do you want me to make a PR with the migration done and schema replaced? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ5FWdWVnrZ99hws6CcCXqoWErSw_ks5sLTP_gaJpZM4ONkX1> .

ryzokuken · 2017-07-06T19:27:00Z

@jywarren No problem. I'll do it right now, all I need to do is rake db:migrate and then copy my schema to the example schema, right?

* Use SecureRandom.uuid for generating per-user tokens * Remove redundant drop_table calls * Run migration and copy database schema

jywarren · 2017-07-06T20:00:40Z

Huh, did u notice there were changes to a few other tables in your schema? I think they're SQLite MySQL differences. Could you remove them? Thanks!

ryzokuken · 2017-07-06T20:07:01Z

@jywarren I will. Just a minute.

* Use SecureRandom.uuid for generating per-user tokens * Remove redundant drop_table calls * Run migrations and update schema

ryzokuken · 2017-07-06T21:01:31Z

@jywarren were they still there? (the sqlite differences)

Strange. I made sure to manually remove them all.

ryzokuken · 2017-07-06T21:05:53Z

@jywarren Nope, here's my schema.rb.example:

add_index "impressions", ["controller_name", "action_name", "ip_address"], :name => "controlleraction_ip_index"
  add_index "impressions", ["controller_name", "action_name", "request_hash"], :name => "controlleraction_request_index"
  add_index "impressions", ["controller_name", "action_name", "session_hash"], :name => "controlleraction_session_index"
  add_index "impressions", ["impressionable_type", "impressionable_id", "ip_address"], :name => "poly_ip_index"
  add_index "impressions", ["impressionable_type", "impressionable_id", "params"], :name => "poly_params_request_index", :unique => false, :length => { "params" => 255 }
  add_index "impressions", ["impressionable_type", "impressionable_id", "request_hash"], :name => "poly_request_index"
  add_index "impressions", ["impressionable_type", "impressionable_id", "session_hash"], :name => "poly_session_index"
  add_index "impressions", ["impressionable_type", "message", "impressionable_id"], :name => "impressionable_type_message_index", :unique => false, :length => { "message" => 255 }
  add_index "impressions", ["user_id"], :name => "index_impressions_on_user_id"

ryzokuken

Let's take care of these two.

ryzokuken · 2017-07-06T21:09:10Z

db/schema.rb.example

@@ -190,26 +190,12 @@ ActiveRecord::Schema.define(:version => 20170208174046) do
  add_index "impressions", ["controller_name", "action_name", "request_hash"], :name => "controlleraction_request_index"
  add_index "impressions", ["controller_name", "action_name", "session_hash"], :name => "controlleraction_session_index"
  add_index "impressions", ["impressionable_type", "impressionable_id", "ip_address"], :name => "poly_ip_index"
-  add_index "impressions", ["impressionable_type", "impressionable_id", "params"], :name => "poly_params_request_index", :unique => false, :length => { "params" => 255 }
+  add_index "impressions", ["impressionable_type", "impressionable_id", "params"], :name => "poly_params_request_index"


This is what went wrong, but this one was done in another commit, I think.

ryzokuken · 2017-07-06T21:09:21Z

db/schema.rb.example

  add_index "impressions", ["impressionable_type", "impressionable_id", "request_hash"], :name => "poly_request_index"
  add_index "impressions", ["impressionable_type", "impressionable_id", "session_hash"], :name => "poly_session_index"
-  add_index "impressions", ["impressionable_type", "message", "impressionable_id"], :name => "impressionable_type_message_index", :unique => false, :length => { "message" => 255 }
+  add_index "impressions", ["impressionable_type", "message", "impressionable_id"], :name => "impressionable_type_message_index"


jywarren · 2017-07-06T21:11:28Z

Can you try reverting those lines? Thanks!

ryzokuken · 2017-07-06T21:12:21Z

@jywarren sure, I will.

* Use SecureRandom.uuid for generating per-user tokens * Remove redundant drop_table calls * Fix database inconsistencies

PublicLabBot · 2017-07-06T22:30:09Z

	1 Warning
⚠️	New migrations added. Please update `schema.rb.example` by overwriting it with a copy of the up-to-date `db/schema.rb`.

	2 Messages
📖	@jywarren Thank you for your pull request! I’m here to help with some tips and recommendations. Please take a look at the list provided and help us review and accept your contribution!
📖	It looks like you haven’t marked all the checkboxes. Help us review and accept your suggested changes by going through the steps one by one. If it is still a ‘Work in progresss’, please include ‘[WIP]’ in the title.

Generated by 🚫 Danger

jywarren · 2017-07-06T22:31:10Z

aha! now this is my error. looking now.

ryzokuken · 2017-07-06T22:31:39Z

@jywarren But we did replace schema.rb.example didn't we?

ryzokuken · 2017-07-06T22:52:57Z

@jywarren the tests need to be changed to match the current schema.

jywarren · 2017-07-07T14:32:28Z

db/migrate/20170704165711_add_user_bio_and_token.rb

@@ -0,0 +1,26 @@
+class AddUserBioAndToken < ActiveRecord::Migration
+  def up
+    add_column :rusers, :bio, :text, limit: 2147483647, default: ''


it's odd; the error I'm seeing is:

Error: test_confirm_user_reset_key_not_visible_on_profile_to_non-admins(UsersControllerTest): ActionView::Template::Error: wrong argument type nil (expected String) app/views/users/profile.html.erb:149:in `to_html'

that's on this line:

<p><small><%= raw auto_link(RDiscount.new(@profile_user.bio).to_html, :sanitize => false) %></small></p>

And @profile_user is a User as of this line: https://github.com/jywarren/plots2/blob/user-bio-and-token/app/controllers/users_controller.rb#L107

So, given this default, it should have a value of '', not nil. What's the deal?

I'm adding a unit test to ensure that a new User record has a non-nil bio.

@jywarren I'll try to look into it.

Oh, okay. Yes, in the migration we set the default bio to '', right?

yes, that's what's confusing!

Apparently default values are not universally supported??? whoa. I'll try setting it manually.

Using rails console?

@jywarren Oh, the fixtures weren't updated?

ugh yeah. i should've done this all at once, locally, but i kept thinking it was a small fix.

@jywarren I'm glad we have this sorted. What next for this PR?

jywarren · 2017-07-07T22:21:52Z

This is good to go! If you can now open a new PR with the comment_controller.rb work to enable posting a comment using only a token, and not just if you're logged in, that'd be great.

ryzokuken · 2017-07-07T22:23:08Z

@jywarren I would. I think I should go to bed now but I definitely will do so tomorrow morning.

jywarren · 2017-07-07T22:43:15Z

good night! Thanks for your hard work as always!

…

On Fri, Jul 7, 2017 at 6:23 PM, Ujjwal Sharma ***@***.***> wrote: @jywarren <https://github.com/jywarren> I would. I think I should go to bed now but I definitely will do so tomorrow morning. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1497 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ2VgTqDRCoUdZoeJbBHb9GZQvUHVks5sLq_NgaJpZM4ONkX1> .

bio field now part of User; status moved off of DrupalUser

4014dfc

jywarren commented Jul 4, 2017

View reviewed changes

jywarren mentioned this pull request Jul 4, 2017

Deprecate Drupal legacy database structures/naming #956

Open

16 tasks

schema.rb

44011ae

Update 20170704165711_add_user_bio_and_token.rb

b849133

Use SecureRandom.uuid for generating per-user tokens (#253)

7092a8a

jywarren added 2 commits July 6, 2017 14:51

schema and tweaks

eb697c6

schema example tweak

d47eae8

Patch token (#254)

7180299

* Use SecureRandom.uuid for generating per-user tokens * Remove redundant drop_table calls * Run migration and copy database schema

Patch token (#255)

21bca11

* Use SecureRandom.uuid for generating per-user tokens * Remove redundant drop_table calls * Run migrations and update schema

ryzokuken reviewed Jul 6, 2017

View reviewed changes

Patch token (#256)

d943b4c

* Use SecureRandom.uuid for generating per-user tokens * Remove redundant drop_table calls * Fix database inconsistencies

Update drupal_users.rb

49f939a

jywarren added 5 commits July 6, 2017 19:00

Update _tags_form.html.erb

dc19d1b

Update user.rb

a97c134

Update 20170704165711_add_user_bio_and_token.rb

5bc7cab

Update schema.rb.example

dab6503

Update profile.html.erb

19e2c16

jywarren commented Jul 7, 2017

View reviewed changes

jywarren added 5 commits July 7, 2017 10:34

Update user_test.rb

29b9f35

Update user.rb

04ea0eb

Update users.yml

d863e57

Update users.yml

237e5d1

Update rusers.yml

61ce717

jywarren merged commit 049bc1b into publiclab:master Jul 7, 2017

ryzokuken added the plotsbot label Aug 23, 2017

bio field now part of User; status moved off of DrupalUser #1497

bio field now part of User; status moved off of DrupalUser #1497

Conversation

jywarren commented Jul 4, 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

jywarren commented Jul 4, 2017 via email

ryzokuken commented Jul 4, 2017 • edited Loading

jywarren commented Jul 4, 2017

ryzokuken commented Jul 4, 2017 • edited Loading

jywarren commented Jul 4, 2017 via email

ryzokuken commented Jul 4, 2017

jywarren commented Jul 4, 2017 via email

ryzokuken commented Jul 4, 2017

jywarren commented Jul 4, 2017 via email

ryzokuken commented Jul 4, 2017

ryzokuken commented Jul 4, 2017

jywarren commented Jul 5, 2017

ryzokuken commented Jul 5, 2017

ryzokuken commented Jul 5, 2017

ryzokuken commented Jul 6, 2017

ryzokuken commented Jul 6, 2017

jywarren commented Jul 6, 2017 via email

ryzokuken commented Jul 6, 2017

jywarren commented Jul 6, 2017

ryzokuken commented Jul 6, 2017

ryzokuken commented Jul 6, 2017

ryzokuken commented Jul 6, 2017

ryzokuken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Jul 6, 2017

ryzokuken commented Jul 6, 2017

PublicLabBot commented Jul 6, 2017 • edited Loading

jywarren commented Jul 6, 2017

ryzokuken commented Jul 6, 2017

ryzokuken commented Jul 6, 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

jywarren commented Jul 7, 2017

ryzokuken commented Jul 7, 2017

jywarren commented Jul 7, 2017 via email

jywarren commented Jul 4, 2017 •

edited

Loading

ryzokuken commented Jul 4, 2017 •

edited

Loading

ryzokuken commented Jul 4, 2017 •

edited

Loading

PublicLabBot commented Jul 6, 2017 •

edited

Loading