Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to Rails 4.2 #2533

Merged
merged 50 commits into from Apr 6, 2018

Conversation

Projects
None yet
8 participants
@Souravirus
Copy link
Member

commented Mar 23, 2018

Related to #1832
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!

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

This looks pretty good to me, to be honest. Almost all changes are minor OR changes to deliver and deliver_now. Fantastic work!

@gauravano
Copy link
Member

left a comment

Really awesome work @Souravirus, Thanks for completing it 💯 🎉

@@ -7,14 +7,14 @@ class NodeInsertExtrasTest < ActionDispatch::IntegrationTest
assert_equal 0, Impression.count

assert_difference 'Impression.count', 1 do
get "wiki/#{nodes(:about).slug}"
get "/wiki/#{nodes(:about).slug}" #gave the complete address changed from wiki to /wiki

This comment has been minimized.

Copy link
@gauravano

gauravano Apr 6, 2018

Member

feel free to remove this comment anytime

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

Yeah @gauravano I will remove these


assert_response :success
end

assert_difference 'nodes(:about).totalviews', 0 do
assert_difference 'Impression.count', 0 do
get "wiki/#{nodes(:about).slug}"
get "/wiki/#{nodes(:about).slug}" #gave the complete address changed from wiki to /wiki

This comment has been minimized.

Copy link
@gauravano

gauravano Apr 6, 2018

Member

this comment can also be removed

@ryzokuken
Copy link
Member

left a comment

Looks really good overall, nice work indeed!

That said, I thought that a few changes were general code improvements and had nothing to do with the Rails 4.2 upgrade. Let me know if I'm wrong, and if I'm not, consider moving them to different PRs.

@@ -65,8 +65,7 @@ def accept
else
@answer.accepted = true
@answer.save
@answer.node.add_tag('answered', @answer.author)

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

I understand the other difference (the addition of deliver_now), but any reason in particular why you'd remove this line?

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

I removed it mistakenly. I would add it again. Thanks!!

@@ -12,6 +12,12 @@ class DrupalContentTypeMap < ActiveRecord::Base
lat_column_name: :lat,
lng_column_name: :lng

before_save :truncate_fields

def truncate_fields

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

Is this some Rails 4.2 specific thing? I just seems like a general improvement to type-assertion, in which case, can't we keep this in another PR instead?

I'm okay either way, but I really like when a PR deals with just one thing, makes things easier to reason about.

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

@ryzokuken screenshot from 2018-04-06 22-13-24
Actually these were added by @jywarren during the rails 4.2 update

This comment has been minimized.

Copy link
@jywarren

jywarren Apr 6, 2018

Contributor

Early in the upgrade process I found I had to do this to get one test to pass, so I believe for some reason this was 4.2 related. Good question!

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

Okay. @jywarren what do you say? I'd rather do a single thing in each PR.

This comment has been minimized.

Copy link
@jywarren

jywarren Apr 6, 2018

Contributor

The ones I added were Rails 4.2 specific, so I'm fine with them being squashed into this. Sometimes with our atypical legacy code we require workarounds to move up Rails versions which may not immediately be apparent from the upgrade checklist but are borne out by the CI tests.

More broadly I am very much in favor of what you're saying -- if things can be merged in separately, then we can refer back to this PR as a good, concise example of a version upgrade, as narrowly defined as possible! In this case, as soon as @Souravirus re-adds the line on line 68 of answers_controller.rb, I think we are good to move forward. Thanks for such a careful read-through!

@@ -106,5 +106,6 @@ class Application < Rails::Application

# Allow mass assignments
config.active_record.whitelist_attributes = false
config.active_record.raise_in_transactional_callbacks = true

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

Again, is this a code improvement which has nothing to do with the upgrade? If yes, I'd love if you cherry-picked this commit into another PR.

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

No @ryzokuken, its a commit regarding to the update
screenshot from 2018-04-06 22-31-54

@@ -10,6 +10,7 @@ test:
encoding: utf8
pool: 5
timeout: 5000
strict: false

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

Same as above

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

This was added by @jywarren in the update process.
Here is the screenshot:
screenshot from 2018-04-06 22-50-45

This comment has been minimized.

Copy link
@jywarren

jywarren Apr 6, 2018

Contributor

Again, this was to get a test to pass. Thanks!

@@ -18,6 +19,7 @@ development:
password:
encoding: utf8
database: publiclaboratory
strict: false

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

Same as above

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

This was added by @jywarren in the update process

@@ -28,3 +30,4 @@ production:
encoding: utf8
pool: 5
timeout: 5000
strict: false

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

Same as above

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

This was added by @jywarren in the update process

# config.log_level = :debug
# Set to `:info` to match the current default, or set to `:debug` to opt-into
# the future default.
config.log_level = :info

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Apr 6, 2018

Member

Same as above

This comment has been minimized.

Copy link
@Souravirus

Souravirus Apr 6, 2018

Author Member

This was added by @jywarren in the update process

@ryzokuken

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

Also, when we merge this, please make sure to squash and merge this.

@Souravirus if possible, once the contents of this PR get approved completely, squash them.

@Souravirus

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

Yeah definitely I will squash them thanks @ryzokuken

@Souravirus

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

Done the changes as suggested by @gauravano and @ryzokuken . Please see these changes @gauravano, @ryzokuken @jywarren. Thanks!!

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

OK -- perfect. I will check with @icarito in a few hours for one final check and we will move ahead with this!

@Souravirus

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

@jywarren jywarren added the ready label Apr 6, 2018

@jywarren jywarren referenced this pull request Apr 6, 2018

Merged

[WIP] Adding Search bar to Maps . #2582 #2585

5 of 5 tasks complete
@sagarpreet-chadha
Copy link
Contributor

left a comment

Wow...Fantastic work !

@jywarren jywarren merged commit 9795328 into publiclab:master Apr 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jywarren

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

This is merged now!!

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

Congratulations @Souravirus and everybody!

@jywarren jywarren referenced this pull request Apr 6, 2018

Closed

initial Rails 4.2 port #1832

2 of 2 tasks complete
@Souravirus

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

Thanks !!

@Souravirus

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2018

@jywarren when it will be updated in the original website?

@Souravirus Souravirus deleted the Souravirus:rails-4.2_errors branch Apr 6, 2018

@jywarren

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

@seafr

This comment has been minimized.

Copy link
Member

commented Apr 7, 2018

@Souravirus congrats on successfully taking on such a big challenge.

@Souravirus

This comment has been minimized.

Copy link
Member Author

commented Apr 7, 2018

thanks @seafr

@icarito icarito referenced this pull request Apr 8, 2018

Merged

Update Paperclip to v5 #2589

@icarito

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Hey great work here, it's live on the publiclab.org website! Thanks for your contribution!

@Souravirus

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2018

Ah! great. Thanks!!

@jywarren jywarren referenced this pull request Feb 26, 2019

Open

Planning Issue: Upgrade rails from 3.2 to rails 6 #305

0 of 20 tasks complete

SrinandanPai pushed a commit to SrinandanPai/plots2 that referenced this pull request May 5, 2019

Upgrade to Rails 4.2 (publiclab#2533)
* initial Rails 4.2 port

* changing over to deliver_now for mails

* updated Gemfile

* truncating fixture field

* truncating fixture field

* nother fixture fix! fix!

* nother fixture fix! fix!

* last try with test dates for maps

* truncation in model

* strict: false in database.yml

* back to before_save filter

* back to before_save filter

* deliver_now

* strict back off...

* strict back off...

* Removed the error of undefined method 'any_instance'

* changed wiki to /wiki because it was inside a integration test

* Added deliver_now in the admin_mailer and node.rb

* added test for AdminMailer.notify_node_moderators in notes_controller_test.rb

* Added test for AdminMailer.notify_moderators_of_spam in admin_controller.rb

* modified the deliver.now in comment_mailer.rb

* Removed the errors related to reset_notify function in password_reset_mailer

* modified the deliver_now in subscription_mailer.rb

* modified the deliver_now in welcome_mailer.rb

* Converted some assert_select to css_select and wrote rails-dom-testing in Gemfile

* Added Gemfile.lock

* Added few corrections

* Removed conflicts

* Modified the Gemfile.lock

* Changed notes_controller

* Corrected some suggestions

* corrected some assert_select in editor_controller_test

* Completed tests of controller_test.rb

* corrected some assert_select

* Removed few assert_select errors

* Removed assert_select errors from notes_controller_test.rb

* Reduced an error and removed all failures in functional

* Removed the errors of tag_controller

* Fixed all the errors and failures in functional folder

* Removed errorfile

* Removed all the errors

* Added html-entities in rails test

* Removed all fails

* Removed some depreciation warnings

* Removed a fail

* Removed some depreciation warnings

* Some changes done as suggested by reviewers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.