[20325][20701] Replace dynamic finders, fix ActiveRecord deprecation warnings #3194

myabc · 2015-06-29T12:39:32Z

💁 This PR doesn't deal with deprecation warnings that are not ActiveRecord related.

Supercedes #3186

https://community.openproject.org/work_packages/20325
https://community.openproject.org/work_packages/20701

TeatroIO · 2015-06-29T12:39:33Z

I've prepared a stage to preview changes. Open stage or view logs.

houndci-bot · 2015-06-29T12:41:42Z

spec/controllers/api/v2/versions_controller_spec.rb

@@ -153,7 +153,7 @@
          let(:child_project) { FactoryGirl.create(:project, parent: project) }
          let(:shared_version) { FactoryGirl.create(:version, project: project, sharing: 'descendants') }

-          before { get :index, ids: shared_version.id.to_s, project_id: child_project.id, format: :json }
+          before do get :index, ids: shared_version.id.to_s, project_id: child_project.id, format: :json end


Line is too long. [108/100]

This patch replaces all dynamic finders, for the sake of consistency, although only some methods are deprecated. See: https://github.com/rails/activerecord-deprecated_finders#active-record-deprecated-finders * Revert some `User#find_by_login` usages in cuke steps accidentally removed in 74228b5. User Story # 20325 Signed-off-by: Alex Coles <[email protected]>

`.find_or_create_by` will call `.first`. Signed-off-by: Alex Coles <[email protected]>

Signed-off-by: Alex Coles <[email protected]>

houndci-bot · 2015-07-07T18:02:24Z

db/migrate/20131024140048_migrate_timelines_options.rb

@@ -128,8 +128,7 @@ def migrate_planning_element_types(timelines_opts, pe_type_id_map, calling_class

    calling_class.contains_none_element = calling_class.contains_none_element? || pe_types.empty?

-    pe_types = pe_types.empty? ? new_ids_of_former_pes
-                               : pe_types.map { |p| pe_type_id_map[p.to_i].to_s }
+    pe_types = pe_types.empty? ? new_ids_of_former_pes : pe_types.map { |p| pe_type_id_map[p.to_i].to_s }


Line is too long. [105/100]

This reverts commit 8e47f60.

NobodysNightmare · 2015-07-08T07:59:32Z

You should see my hint on calling Project.allowed_to_condition, includes and references as a more general advice. I've seen this pattern with other methods too, where the caller uses some kind of condition he really doesn't know, but for some reason starts to include and reference fancy tables. Sometimes acompanied with a comment that excuses for this.

I think since the introduction of references only amplifies that (already existing) problem, we should at least start to look for a way out for the worst offenders.

NobodysNightmare · 2015-07-08T08:46:43Z

Pew... reviewed until the latest commit:

Revert "DRY WorkPackageSchema#available_custom_fields spec" … 0436446

Looking nice (and tedious) :)

Reported as issue upstream: rubocop/rubocop#2021 [ci skip] Signed-off-by: Alex Coles <[email protected]>

[ci skip] Signed-off-by: Alex Coles <[email protected]>

Missed by rubocop auto-correct. [ci skip] Signed-off-by: Alex Coles <[email protected]>

houndci-bot · 2015-07-08T09:44:00Z

app/models/work_package/pdf_exporter.rb

-        current_language.to_s.downcase == 'ja'    ||
-        current_language.to_s.downcase == 'zh'    ||
-        current_language.to_s.downcase == 'zh-tw' ||
+    if  current_language.to_s.downcase == 'ko' || current_language.to_s.downcase == 'ja' || current_language.to_s.downcase == 'zh' || current_language.to_s.downcase == 'zh-tw' ||


Line is too long. [178/100]
Unnecessary spacing detected.

…namic-finders Signed-off-by: Alex Coles <[email protected]>

[ci skip] Signed-off-by: Alex Coles <[email protected]>

houndci-bot · 2015-07-09T09:36:03Z

app/models/work_package/pdf_exporter.rb

-        current_language.to_s.downcase == 'ja'    ||
-        current_language.to_s.downcase == 'zh'    ||
-        current_language.to_s.downcase == 'zh-tw' ||
+    if  current_language.to_s.downcase == 'ko' || current_language.to_s.downcase == 'ja' || current_language.to_s.downcase == 'zh' || current_language.to_s.downcase == 'zh-tw' ||


Line is too long. [178/100]
Unnecessary spacing detected.

Signed-off-by: Alex Coles <[email protected]>

myabc · 2015-07-09T09:56:22Z

@NobodysNightmare

Pew... reviewed until the latest commit:

Thanks!

Looking nice (and tedious) :)

Yes, sure is tedious.

I think I've now dealt with all of your comments, with the exception of reordering references in query chains. As we already discussed, there may be some references that can be removed once we're on Rails 4.1.

tl; dr: sometimes an automatic tool just fails to do the right thing
I am not sure how we want to deal with this problem in general. Arguably these changes by you make the overall situation better, while introducing collateral damage. That is why I am inclined to merge those changes regardless.
But I don't feel like we will be able to keep "100% rubocop -a compatibility" in the long term. I don't know if this is a problem...

I reported one of the issues upstream (rubocop/rubocop#2021). I tend to be optimistic though, and think rubocop can only get better at linting/auto-correcting code. It's annoying though that Hound is still on a slightly older version of rubocop and that there are some discrepancies in configurations.

ulferts · 2015-07-10T07:47:26Z

app/controllers/api/v2/planning_elements_controller.rb

@@ -164,7 +165,7 @@ def find_all_projects_by_project_id
          # WTF. Why do we completely skip rewiring in this case and always provide parent_ids?
          # This is totally inconistent.
          identifiers = params[:ids].split(/,/).map(&:strip)
-          @planning_elements = WorkPackage.visible(User.current).find_all_by_id(identifiers)
+          @planning_elements = WorkPackage.visible(User.current).where(id: identifiers)


This now returns an ActiveRecord::Relation. Don't know if that is a problem, though.

@ulferts that shouldn't be a problem.

ulferts · 2015-07-10T12:22:39Z

@myabc I am done with my review and once I received statements to my comments, willing to merge this PR.

Thanks for the huge amount of work you have put in here!

…ynamic-finders [20325][20701] Replace dynamic finders, fix ActiveRecord deprecation warnings

myabc mentioned this pull request Jun 29, 2015

[20325] Replace dynamic finders #3186

Closed

houndci-bot reviewed Jun 29, 2015
View reviewed changes

myabc mentioned this pull request Jun 29, 2015

[20043] Upgrade to Rails 4.1.x #3197

Closed

myabc added the feature label Jun 29, 2015

myabc added 5 commits June 30, 2015 11:50

Rework DeletedUser.first to avoid stack overflow

16720d2

`.find_or_create_by` will call `.first`. Signed-off-by: Alex Coles <[email protected]>

Fix syntax (w/Rubocop) in (Rails) controllers

ec1bb39

Signed-off-by: Alex Coles <[email protected]>

Fix syntax (w/Rubocop) in (Rails) models

df51b6e

Signed-off-by: Alex Coles <[email protected]>

Fix syntax (w/Rubocop) in specs

ce93701

Signed-off-by: Alex Coles <[email protected]>

houndci-bot reviewed Jul 7, 2015
View reviewed changes

Revert "DRY WorkPackageSchema#available_custom_fields spec"

0436446

This reverts commit 8e47f60.

myabc added 3 commits July 8, 2015 11:04

Fix bad rubocop auto-correct in journal_manager.rb

8fd5357

Reported as issue upstream: rubocop/rubocop#2021 [ci skip] Signed-off-by: Alex Coles <[email protected]>

Indent fix in acts_as_searchable

1bcbad9

[ci skip] Signed-off-by: Alex Coles <[email protected]>

Fix block delimiter in MailHandler

bbd6943

Missed by rubocop auto-correct. [ci skip] Signed-off-by: Alex Coles <[email protected]>

houndci-bot reviewed Jul 8, 2015
View reviewed changes

myabc added 2 commits July 8, 2015 17:37

Merge branch 'feature/rails4' into feature/20325-rails4-deprecated-dy…

59bb031

…namic-finders Signed-off-by: Alex Coles <[email protected]>

Remove obsolete TODO in VersionsController spec

985a15f

[ci skip] Signed-off-by: Alex Coles <[email protected]>

houndci-bot reviewed Jul 9, 2015
View reviewed changes

Without message chain, prefer #receive in spec

1d1d562

Signed-off-by: Alex Coles <[email protected]>

myabc mentioned this pull request Jul 9, 2015

Implement Project.allowed_to, returning an ActiveRecord::Relation #3225

Merged

ulferts reviewed Jul 10, 2015
View reviewed changes

ulferts added a commit that referenced this pull request Jul 10, 2015

Merge pull request #3194 from myabc/feature/20325-rails4-deprecated-d…

c13aa00

…ynamic-finders [20325][20701] Replace dynamic finders, fix ActiveRecord deprecation warnings

ulferts merged commit c13aa00 into opf:feature/rails4 Jul 10, 2015

myabc deleted the feature/20325-rails4-deprecated-dynamic-finders branch July 13, 2015 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[20325][20701] Replace dynamic finders, fix ActiveRecord deprecation warnings #3194

[20325][20701] Replace dynamic finders, fix ActiveRecord deprecation warnings #3194

myabc commented Jun 29, 2015

TeatroIO commented Jun 29, 2015

houndci-bot Jun 29, 2015

houndci-bot Jul 7, 2015

NobodysNightmare commented Jul 8, 2015

NobodysNightmare commented Jul 8, 2015

houndci-bot Jul 8, 2015

houndci-bot Jul 9, 2015

myabc commented Jul 9, 2015

ulferts Jul 10, 2015

myabc Jul 10, 2015

ulferts commented Jul 10, 2015

[20325][20701] Replace dynamic finders, fix ActiveRecord deprecation warnings #3194

[20325][20701] Replace dynamic finders, fix ActiveRecord deprecation warnings #3194

Conversation

myabc commented Jun 29, 2015

TeatroIO commented Jun 29, 2015

houndci-bot Jun 29, 2015

Choose a reason for hiding this comment

houndci-bot Jul 7, 2015

Choose a reason for hiding this comment

NobodysNightmare commented Jul 8, 2015

NobodysNightmare commented Jul 8, 2015

houndci-bot Jul 8, 2015

Choose a reason for hiding this comment

houndci-bot Jul 9, 2015

Choose a reason for hiding this comment

myabc commented Jul 9, 2015

ulferts Jul 10, 2015

Choose a reason for hiding this comment

myabc Jul 10, 2015

Choose a reason for hiding this comment

ulferts commented Jul 10, 2015