Notifications field added while creating users with omniauth #3158

SidharthBansal · 2018-07-26T17:49:00Z

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with rake test
code is in uniquely-named feature branch and has no merge conflicts
PR is descriptively titled
PR body includes fixes #0000-style reference to original issue #
ask @publiclab/reviewers for help, in a comment below

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If tests do fail, click on the red X to learn why by reading the logs.

Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software

We have a loose schedule of reviewing and pulling in changes every Tuesday and Friday, and publishing changes on Fridays.

Thanks!

plotsbot · 2018-07-26T17:59:08Z

	2 Warnings
⚠️	New migrations added. Please update `schema.rb.example` by overwriting it with a copy of the up-to-date `db/schema.rb`. Also, be aware to preserve the MySQL-specific conditions for full-text indices.
⚠️	It looks like you merged from master in this pull request. Please rebase to get rid of the merge commits – you may want to rewind the master branch and rebase instead of merging in from master, which can cause problems when accepting new code!

	2 Messages
📖	@SidharthBansal 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! And don’t be discouraged if you see errors – we’re here to help.
📖	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

SidharthBansal · 2018-07-31T12:48:16Z

@jywarren we migrated users instead of rusers in #3098. Can you please rollback the db for changes of #3098 before merging this one? Sorry for the inconvenience.

SidharthBansal · 2018-07-31T14:24:40Z

jywarren · 2018-07-31T15:43:11Z

oh no, oops! So, we actually have to add a reversed migration, rather than rolling back, now that it's in the commit history. Can you open a PR with undoing the previous change and adding the new one?

…

On Tue, Jul 31, 2018 at 8:48 AM Sidharth Bansal ***@***.***> wrote: @jywarren <https://github.com/jywarren> we migrated users instead of rusers in #3098 <#3098>. Can you please rollback the db for changes of #3098 <#3098> before merging this one? Sorry for the inconvenience. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3158 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJyA6PHkxLdfg3rpWQAGY_kOovgR3ks5uMFIQgaJpZM4ViRmd> .

SidharthBansal · 2018-07-31T17:51:52Z

Hi @jywarren I have added the migration for removal of password_checker field in this pr. Can you please review it?

SidharthBansal · 2018-07-31T17:56:09Z

I think we are getting octokit error again. Maybe it is due to change of key as it happened earlier

SidharthBansal · 2018-08-01T01:23:15Z

Can we please ignore codeclimate errors? They are for deeply nested if-else.

jywarren

Couple small changes! Thanks!!!!

jywarren · 2018-08-01T02:13:36Z

db/migrate/20180731124028_add_password_checker_to_rusers.rb

@@ -0,0 +1,5 @@
+class AddPasswordCheckerToRusers < ActiveRecord::Migration[5.2]
+  def change
+    add_column :rusers, :password_checker, :integer, default: 0


These could actually be in the same migration!

Sorry, I could not understand what I need to change.

There are two migrations. First is to delete password_checker from Users.
Second is to add password_checker to RUsers.

Oh just that you can perform both steps in the same migration file. They needn't be separated... Although it's not a big deal, over the lifetime of the project I think we should try not to have too many migration files. Mind changing this? Thanks!

I will change it

Thanks for explanation

Can you please add link to the code for example? I tried a lot searching on net and on plots2 migration files. We can have different dbms queries on same table in single migration. But I could not find a migration with two different tables involved. In this pr, we are involved with two tables Rusers(removal of password checker) & Users(addition of password checker).

I believe it'll work to do:

def change remove_column :users, :password_checker, :integer add_column :rusers, :password_checker, :integer, default: 0 end

I don't have an example but have you tried it? I think the table name is just specified in the one line, no? But honestly it's not a big deal, just a slight preference, so if you find that it doesn't work, 2 files is fine!

Thanks a lot. I was thinking other type of solution which was giving errors.
Thanks it worked.

jywarren · 2018-08-01T02:14:46Z

app/controllers/user_sessions_controller.rb

+      u = User.find_by(username: username)
+      if u && u.password_checker != 0
+        n = u.password_checker
+        hash = { 1 => "Facebook", 2 => "Github", 3 => "Google", 4 => "Twitter"  }


Is it possible to use the style 1: "Facebook" here?

Hi I tried but it is giving syntax error. I know that we should use colons instead of hash rockets. But this is not working here. I guess, we use colons when we use symbols --> value.
Thanks

I can't make integers as symbols as at another place I am using the reverse hash. In reverse hash I am mapping providers with integer values.

ok! thanks for trying!

jywarren · 2018-08-01T02:15:38Z

Also where could we helpfully explain the usage of password checker?

SidharthBansal · 2018-08-01T02:25:10Z

I have sent the documentation.

SidharthBansal · 2018-08-01T15:34:33Z

@jywarren can you please review this?
Thanks

jywarren · 2018-08-01T20:36:11Z

This looks good - are you confident that we should merge? Thanks!

SidharthBansal · 2018-08-02T00:55:10Z

Yes
Thanks

…ab#3158) * Notifications field added in users * Rails migration rollback and new migration added * Migrations corrected * Added documentation * Two migrations merged into one

Notifications field added in users

5231161

ghost assigned SidharthBansal Jul 26, 2018

ghost added the in progress label Jul 26, 2018

SidharthBansal added the help wanted requires help by anyone willing to contribute label Jul 26, 2018

Rails migration rollback and new migration added

23fb2de

SidharthBansal force-pushed the notification branch from 0bea481 to 23fb2de Compare July 31, 2018 14:28

SidharthBansal added ready and removed help wanted requires help by anyone willing to contribute in progress labels Jul 31, 2018

SidharthBansal added this to the OAuth milestone Jul 31, 2018

Migrations corrected

d4ec396

jywarren requested changes Aug 1, 2018

View reviewed changes

Added documentation

44ada75

SidharthBansal force-pushed the notification branch from 57dadcb to 44ada75 Compare August 1, 2018 02:53

Two migrations merged into one

38e0d09

SidharthBansal force-pushed the notification branch from b76ebf7 to 38e0d09 Compare August 1, 2018 15:07

Merge branch 'master' into notification

397cde9

jywarren approved these changes Aug 1, 2018

View reviewed changes

jywarren merged commit 010bb27 into publiclab:master Aug 2, 2018

ghost removed the ready label Aug 2, 2018

SidharthBansal added the summer-of-code label Aug 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications field added while creating users with omniauth #3158

Notifications field added while creating users with omniauth #3158

SidharthBansal commented Jul 26, 2018 •

edited

Loading

plotsbot commented Jul 26, 2018 •

edited

Loading

SidharthBansal commented Jul 31, 2018

SidharthBansal commented Jul 31, 2018

jywarren commented Jul 31, 2018 via email

SidharthBansal commented Jul 31, 2018

SidharthBansal commented Jul 31, 2018

SidharthBansal commented Aug 1, 2018

jywarren left a comment

jywarren Aug 1, 2018

SidharthBansal Aug 1, 2018

SidharthBansal Aug 1, 2018

jywarren Aug 1, 2018

SidharthBansal Aug 1, 2018

SidharthBansal Aug 1, 2018

SidharthBansal Aug 1, 2018

jywarren Aug 1, 2018

SidharthBansal Aug 1, 2018

jywarren Aug 1, 2018

SidharthBansal Aug 1, 2018 •

edited

Loading

SidharthBansal Aug 1, 2018

jywarren Aug 1, 2018

jywarren commented Aug 1, 2018

SidharthBansal commented Aug 1, 2018 •

edited

Loading

SidharthBansal commented Aug 1, 2018

jywarren commented Aug 1, 2018

SidharthBansal commented Aug 2, 2018

Notifications field added while creating users with omniauth #3158

Notifications field added while creating users with omniauth #3158

Conversation

SidharthBansal commented Jul 26, 2018 • edited Loading

plotsbot commented Jul 26, 2018 • edited Loading

SidharthBansal commented Jul 31, 2018

SidharthBansal commented Jul 31, 2018

jywarren commented Jul 31, 2018 via email

SidharthBansal commented Jul 31, 2018

SidharthBansal commented Jul 31, 2018

SidharthBansal commented Aug 1, 2018

jywarren 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

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

SidharthBansal Aug 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jywarren commented Aug 1, 2018

SidharthBansal commented Aug 1, 2018 • edited Loading

SidharthBansal commented Aug 1, 2018

jywarren commented Aug 1, 2018

SidharthBansal commented Aug 2, 2018

SidharthBansal commented Jul 26, 2018 •

edited

Loading

plotsbot commented Jul 26, 2018 •

edited

Loading

SidharthBansal Aug 1, 2018 •

edited

Loading

SidharthBansal commented Aug 1, 2018 •

edited

Loading