Ruby upgrade to 2.4.4

[grvsachdeva]

Fixes #319

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

• tests pass -- rake test
• code has been rebased on top of latest master (check if another pull request was added recently, and please rebase)
• pull request is descriptively named and, if possible, multiple commits squashed if they're smaller changes

Thanks!

[welcome]

Thanks for opening this pull request! Dangerbot will test out your code and reply in a bit with some pointers and requests.
There may be some errors, but don't worry! We're here to help! 👍🎉😄

[grvsachdeva]

Hi @icarito @jywarren , can you please help in completing this upgrade? Also, I think we should upgrade to ruby 2.4.4 so contributors will not face any issue while switching from plots2 to mapknitter.

Thanks!

[jywarren]

Hmm, modeling on https://github.com/publiclab/plots2/blob/master/Dockerfile...

[jywarren]

Hmm... /bin/sh: 1: npm: not found

[jywarren]

Aha! Running :-)

Error: test_should_get_profile(UsersControllerTest): TypeError: ActiveSupport::Duration can't be coerced into Integer

Could do with the fixtures timestamp attributes. We should check how they're generated in plots2, say in the User model.

[jywarren]

Maybe here...

mapknitter/test/fixtures/maps.yml

Line 10 in 20d887f

created_at: <%= Time.now %>

[jywarren]

Hmm, plots2 does the same:

  created_at: <%= Time.now %>

[jywarren]

https://stackoverflow.com/questions/42083737/gem-load-error-is-activesupportduration-cant-be-coerced-into-integer -- ok, can we try running this locally and see what exact line this happens on?

[jywarren]

I bet we're passing a time range, or maybe a gem is, and the newer versions of Ruby can't turn it into an integer somewhere... maybe a time conversion. It's possible we could resolve by updating one or our gems...

[jywarren]

Maybe @Souravirus has an insight, too? He's done a lot of such upgrades :-)

[sashadev-sky]

@jywarren @gauravano I was doing some research for another issue I am opening right now and I just stumbled on this on the MySQL site:

"Removal of MAXDB means that the TIMESTAMP data type for CREATE TABLE or ALTER TABLE is no longer treated as DATETIME."

I think this could be the cause? I will not be able to check until for sure later tonight but I hope its helpful!

When I run rake test this test fails as an ActiveRecord Error:

test_should_get_profile(UsersControllerTest): ActiveRecord::StatementInvalid: Mysql2::Error: Incorrect datetime value: '2019-02-07 12:10:44 UTC' for column 'remember_token_expires_at'

[jywarren]

oh cool - is that the only one that fails or is that just representative? We should check where we may be actually setting that value and see if we need to add or remove some kind of formatting at the moment of insertion. Thanks!

…

On Wed, Feb 6, 2019 at 9:52 AM Sasha Boginsky ***@***.***> wrote: @jywarren <https://github.com/jywarren> @gauravano <https://github.com/gauravano> I was doing some research for another issue I am opening right now and I just stumbled on this on the MySQL site <https://dev.mysql.com/doc/refman/8.0/en/upgrading-from-previous-series.html#upgrade-data-dictionary-changes> : "Removal of MAXDB means that the TIMESTAMP data type for CREATE TABLE or ALTER TABLE is no longer treated as DATETIME." I think this could be the cause? I will not be able to check until for sure later tonight but I hope its helpful! When I run rake test this test fails as an ActiveRecord Error: test_should_get_profile(UsersControllerTest): ActiveRecord::StatementInvalid: Mysql2::Error: Incorrect datetime value: '2019-02-07 12:10:44 UTC' for column 'remember_token_expires_at' — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ2E4-PCYNAo4KFW96NKp_4J7UhXZks5vKuwegaJpZM4abHL-> .

[sashadev-sky]

there are a few small formatting issues. I updated them and will make pull request shortly 👍

It was fixtures you were right

[jywarren]

@sashadev-sky once we merge #323, do you think this will work once rebased?

[sashadev-sky]

@jywarren I updated my local repo to use ruby 2.4.4 and ran the tests and I am now getting those errors. So I guess that was not the fix. But since I have it locally setup already I will see if I can try to fix that as well, change my ruby v back to 2.1.1, make sure everything still works then push another commit onto my current PR?

[sashadev-sky]

@jywarren the issue was because Rails 3.2 and Ruby 2.4 are not compatible and lies in the ActiveSupport library. This incompatibility was fixed in a later versions of Rails.

I don't know if there is interest in updating the Rails version as well, but in the meantime, I added two more files in config/initializers and there are no more errors. I just pushed them to my PR #323 and will make another commit with notes added to them shortly. @gauravano

• ActiveSupport::Duration fix: see here
• Arel::Visitor fix: see here

[Souravirus]

Hi @jywarren @gauravano and @sashadev-sky, I am ready to help in every possible way. Just ping me I will be available.
Thank you !!

[sashadev-sky]

@gauravano @jywarren what is the status on this update? I was thinking to open a PR to update mySQL after it

[grvsachdeva]

hey @sashadev-sky, I will trigger the build again. Let's see how it goes now.

[grvsachdeva]

@jywarren @sashadev-sky try this branch locally and please review this PR. Thanks!

[sashadev-sky]

@gauravano I will check this out today thank you :)

[sashadev-sky]

@gauravano I ran your PR locally and this worked for me! 👍

Just one thing to maybe add, even though it is not specifically Ruby it doesn't seem like it needs its own PR:
mapknitter/config/initializers/new_rails_defaults.rb

this file says it could have been removed a while ago (with Rails 3). I commented it out and there was no difference in setup for me. Should we comment it out and change the file name to maybe old_rails_defaults.rb?

[sashadev-sky]

@gauravano @jywarren @Souravirus I will be opening an issue to update mySQL next. I have gauravano's PR here to guide me but is there any specific documentation that I should be following?

[jywarren]

This seems super, and if you want to merge after removing mapknitter/config/initializers/new_rails_defaults.rb i'd be fine with that!

[sashadev-sky]

@jywarren what is the process for merging? Do you just click "Squash and merge" in this case and your good to go?

[jywarren]

Yes, that's right!

…

On Tue, Feb 19, 2019, 5:51 PM Sasha Boginsky ***@***.*** wrote: @jywarren <https://github.com/jywarren> what is the process for merging? Do you just click "Squash and merge" in this case and your good to go? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#320 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ2Y4hH9nFsNJWAsTuiMGndHm1xATks5vPKoAgaJpZM4abHL-> .

[welcome]

Congrats on merging your first pull request! 🙌🎉⚡️
Your code will likely be published to https://mapknitter.org in the next few days.
In the meantime, can you tell us your Twitter handle so we can thank you properly?
Now that you've completed this, you can help someone else take their first step!
See: Public Lab's coding community!

[jywarren]

I went ahead and merged this but @sashadev-sky if you want to remove that file in a new PR, that'd be great!

[jywarren]

Thanks both of you, this is great.

[sashadev-sky]

@jywarren I’ll go ahead and do that for the mySQL update

…

On Feb 20, 2019, at 5:02 PM, Jeffrey Warren ***@***.***> wrote: Thanks both of you, this is great. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.