Implement search by pattern for Mapping Rules #1239

duduribeiro · 2019-09-25T00:42:15Z

This commit implements a search by pattern for mapping rules. It
includes a search field in the mapping rules page for both Backend
APIs and Services. It also adds an index for ProxyRule class using
Sphinx.

thomasmaas · 2019-09-25T06:24:46Z

Screen capture?

duduribeiro · 2019-09-25T17:40:25Z

@thomasmaas ☝️

duduribeiro · 2019-09-25T18:00:35Z

Martouta

👏 🥇 💪

Needs some polishing :)

test/unit/proxy_rule_test.rb

app/models/proxy_rule.rb

Martouta · 2019-09-26T07:56:31Z

app/models/proxy_rule.rb

@@ -25,6 +28,16 @@ class ProxyRule < ApplicationRecord

  ALLOWED_HTTP_METHODS = %w[GET POST DELETE PUT PATCH HEAD OPTIONS].freeze

+  scope :by_query, ->(query, owner_type, owner_id) do
+    return if query.blank?


Suggested change

return if query.blank?

return none if query.blank?

Not sure if it will work, but then this way we return the same time and we can chain scopes... so it won't be null anymore. Btw, lambda checks that the params are passed so 'query' should never be nil 🤔 so imo we shouldn't even have this checking the type at all

My first shoot was returning a none relation. But it doesn't work because it ignores prepended and postponed relations in the query. eg:

with return none ..

[1] pry(main)> ProxyRule.by_query('', 'Proxy', 22).where(position: 2).to_sql => ""

it returned the empty relation and ignored the where position.

with return nil

[1] pry(main)> ProxyRule.by_query('', 'Proxy', 22).where(position: 2).to_sql => "SELECT `proxy_rules`.* FROM `proxy_rules` WHERE `proxy_rules`.`position` = 2"

it skipped the where(id: ..) and made the where(position: ) one.

This could be a query see app/queries I prefer not mixing scopes with other stuffs.

app/models/proxy_rule.rb

app/workers/index_proxy_rule_worker.rb

app/views/shared/proxy_rules/_table.html.slim

codecov · 2019-09-26T13:41:31Z

Codecov Report

Merging #1239 into master will increase coverage by 5.24%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1239      +/-   ##
==========================================
+ Coverage   86.29%   91.54%   +5.24%     
==========================================
  Files        2139     2265     +126     
  Lines       66794    73359    +6565     
==========================================
+ Hits        57640    67154    +9514     
+ Misses       9154     6205    -2949

Impacted Files	Coverage Δ
app/controllers/api/proxy_rules_controller.rb	`90.32% <100%> (+38.59%)`	⬆️
app/indices/proxy_rules_index.rb	`100% <100%> (ø)`
app/workers/index_proxy_rule_worker.rb	`100% <100%> (ø)`
...der/admin/backend_apis/mapping_rules_controller.rb	`52.17% <100%> (+7.17%)`	⬆️
app/models/proxy_rule.rb	`98.73% <100%> (+0.12%)`	⬆️
app/controllers/proxy_rule_shared_controller.rb	`100% <100%> (+40%)`	⬆️
config/routes.rb	`98.06% <100%> (ø)`	⬆️
app/lib/csv/applications_exporter.rb	`16% <0%> (-84%)`	⬇️
...ce_discovery/import_cluster_definitions_service.rb	`32.3% <0%> (-67.7%)`	⬇️
app/workers/audited_worker.rb	`42.85% <0%> (-57.15%)`	⬇️
... and 880 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c00e32...3745336. Read the comment docs.

Martouta · 2019-09-26T23:54:12Z

app/workers/application_job.rb

@@ -0,0 +1,4 @@
+# frozen_string_literal: true
+
+class ApplicationJob < ActiveJob::Base


😂 😂 😂 😂 😂
Cadu 1 - RuboCop 0

Martouta · 2019-09-26T23:57:58Z

features/support/env.rb

@@ -36,6 +36,10 @@ def initialize(a,b,c)

 transactional = non_transactional.map {|t| "~#{t}" }

+Before do
+  IndexProxyRuleWorker.stubs(:perform_later)


@Martouta
It is run after_commit, so all tests with disable_transactional_fixtures and involving Proxy (basically all, because you create an account + service + proxy) will trigger indexation of Proxy.

We wanted to use ActiveJob::TestHelper perform_enqueued_jobs but it does not have the :only options in Rails 4.2, we need to update to rails 5.x (see the monkey patch)

And anyway it is big task to change it everywhere, so it is acceptable to have it now
We need to move all workers to ActiveJob, not the purpose of this PR

yeah, cucumber enables Sidekiq.inline! for all features, without this, all cucumber tests fails

hallelujah · 2019-09-27T07:49:08Z

test/monkey_patches/active_job_test_helper.rb

+# TODO: Remove this Monkey patch after we move to Rails 5.
+# This is used to ensure that we run only a specific job in a Test.
+# It was extracted from
+# https://github.com/rails/rails/blob/fc5dd0b85189811062c85520fd70de8389b55aeb/activejob/lib/active_job/test_helper.rb#L368


This is acceptable! Really :)

hallelujah

Better idea not to mix models scopes and search queries

Martouta · 2019-09-27T09:28:10Z

Since you are going to push again anyway, personally I'd appreciate if the update/install of Gems could be a separate commit before the actual changes in the code 😄

This commit upgrades thinking sphinx gem. The new version added the ability to filter using string types, avoinding us to save it using a CRC32 version of the string. This commit also upgrades minitest.

Martouta

Amazing job! 👏 🥇 💪

app/controllers/proxy_rule_shared_controller.rb

hallelujah · 2019-09-27T17:21:01Z

app/queries/proxy_rule_query.rb

+    scope = scope.order_by(@sort, @direction).includes(:metric)
+    return scope if query.blank?
+    options = {
+      ids_only: true, star: true, per_page: 1_000_000, ignore_scopes: true,


1 million per page? Are you sure?

yeah.. don't make sense haha.. will add the same maximum as we paginate.

hallelujah · 2019-09-27T17:54:19Z

app/queries/proxy_rule_query.rb

+    scope = scope.order_by(@sort, @direction).includes(:metric)
+    return scope if query.blank?
+    options = {
+      ids_only: true, star: true, per_page: 20, ignore_scopes: true,


Ouch I dis not check correctly
The per page does not use the variable in. Initialize

This commit implements a search by pattern for mapping rules. It includes a search field in the mapping rules page for both Backend APIs and Services. It also adds an index for ProxyRule class using Sphinx.

duduribeiro added the APIAP label Sep 25, 2019

duduribeiro requested review from thomasmaas, hallelujah, guicassolato, macejmic and Martouta September 25, 2019 00:42

duduribeiro self-assigned this Sep 25, 2019

duduribeiro force-pushed the search_mapping_rules branch 4 times, most recently from 681a8b1 to 3c11f33 Compare September 26, 2019 00:50

Martouta previously approved these changes Sep 26, 2019

View reviewed changes

Martouta reviewed Sep 26, 2019

View reviewed changes

test/unit/proxy_rule_test.rb Outdated Show resolved Hide resolved

Martouta reviewed Sep 26, 2019

View reviewed changes

test/unit/proxy_rule_test.rb Outdated Show resolved Hide resolved

Martouta reviewed Sep 26, 2019

View reviewed changes

app/models/proxy_rule.rb Outdated Show resolved Hide resolved

Martouta reviewed Sep 26, 2019

View reviewed changes

app/models/proxy_rule.rb Outdated Show resolved Hide resolved

Martouta reviewed Sep 26, 2019

View reviewed changes

app/workers/index_proxy_rule_worker.rb Outdated Show resolved Hide resolved

thomasmaas reviewed Sep 26, 2019

View reviewed changes

app/views/shared/proxy_rules/_table.html.slim Outdated Show resolved Hide resolved

duduribeiro force-pushed the search_mapping_rules branch from 3c11f33 to 3745336 Compare September 26, 2019 13:41

Martouta self-requested a review September 26, 2019 13:50

duduribeiro force-pushed the search_mapping_rules branch 3 times, most recently from d6384bf to 04017d5 Compare September 26, 2019 19:37

Martouta reviewed Sep 26, 2019

View reviewed changes

hallelujah reviewed Sep 27, 2019

View reviewed changes

hallelujah suggested changes Sep 27, 2019

View reviewed changes

Upgrade thinking-sphinx gem to 3.4.2

d9833b5

This commit upgrades thinking sphinx gem. The new version added the ability to filter using string types, avoinding us to save it using a CRC32 version of the string. This commit also upgrades minitest.

duduribeiro force-pushed the search_mapping_rules branch 3 times, most recently from e348ef0 to e9c43b7 Compare September 27, 2019 14:57

Martouta previously approved these changes Sep 27, 2019

View reviewed changes

hallelujah reviewed Sep 27, 2019

View reviewed changes

app/controllers/proxy_rule_shared_controller.rb Outdated Show resolved Hide resolved

thomasmaas previously approved these changes Sep 27, 2019

View reviewed changes

duduribeiro dismissed stale reviews from thomasmaas and Martouta via dd257ec September 27, 2019 17:00

duduribeiro force-pushed the search_mapping_rules branch from e9c43b7 to dd257ec Compare September 27, 2019 17:00

hallelujah reviewed Sep 27, 2019

View reviewed changes

duduribeiro force-pushed the search_mapping_rules branch from dd257ec to 58ef668 Compare September 27, 2019 17:39

hallelujah approved these changes Sep 27, 2019

View reviewed changes

hallelujah suggested changes Sep 27, 2019

View reviewed changes

Implement search by pattern for Mapping Rules

4c62160

This commit implements a search by pattern for mapping rules. It includes a search field in the mapping rules page for both Backend APIs and Services. It also adds an index for ProxyRule class using Sphinx.

duduribeiro force-pushed the search_mapping_rules branch from 58ef668 to 4c62160 Compare September 27, 2019 17:57

hallelujah approved these changes Sep 27, 2019

View reviewed changes

duduribeiro merged commit a9e62ca into master Sep 27, 2019

duduribeiro deleted the search_mapping_rules branch September 27, 2019 18:33

mayorova mentioned this pull request Aug 29, 2022

THREESCALE-7327: Fix missing pagination links on mapping rules search results #3044

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement search by pattern for Mapping Rules #1239

Implement search by pattern for Mapping Rules #1239

duduribeiro commented Sep 25, 2019

thomasmaas commented Sep 25, 2019

duduribeiro commented Sep 25, 2019

duduribeiro commented Sep 25, 2019

Martouta left a comment

Martouta Sep 26, 2019

duduribeiro Sep 26, 2019

hallelujah Sep 27, 2019 •

edited

codecov bot commented Sep 26, 2019

Martouta Sep 26, 2019

Martouta Sep 26, 2019

hallelujah Sep 27, 2019 •

edited

duduribeiro Sep 27, 2019

hallelujah Sep 27, 2019

hallelujah left a comment

Martouta commented Sep 27, 2019

Martouta left a comment

hallelujah Sep 27, 2019

duduribeiro Sep 27, 2019

hallelujah Sep 27, 2019

		@@ -0,0 +1,4 @@
		# frozen_string_literal: true

		class ApplicationJob < ActiveJob::Base

Implement search by pattern for Mapping Rules #1239

Implement search by pattern for Mapping Rules #1239

Conversation

duduribeiro commented Sep 25, 2019

thomasmaas commented Sep 25, 2019

duduribeiro commented Sep 25, 2019

duduribeiro commented Sep 25, 2019

Martouta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallelujah Sep 27, 2019 • edited

Choose a reason for hiding this comment

codecov bot commented Sep 26, 2019

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallelujah Sep 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallelujah left a comment

Choose a reason for hiding this comment

Martouta commented Sep 27, 2019

Martouta 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

hallelujah Sep 27, 2019 •

edited

hallelujah Sep 27, 2019 •

edited