[Mapping Rules] Sorting mapping rules #530

didierofrivia · 2019-01-22T06:51:26Z

Depends on #658

What this PR does / why we need it:

Feature: Sort mapping rules + last called

Which issue(s) this PR fixes

fixes https://issues.jboss.org/browse/THREESCALE-1787

Verification steps

Visit APIcast configuration page
Play with the order of the mapping rules
Profit!

Special notes for your reviewer:

Done the easiest way
This is meant to be removed in the near future favoring an entire page craft in React using APIcast json schema config.

TODO:

Fixing some bugs
Cuke
Rails part

codecov · 2019-01-23T09:09:33Z

Codecov Report

Merging #530 into master will decrease coverage by <.01%.
The diff coverage is 98.7%.

@@            Coverage Diff             @@
##           master     #530      +/-   ##
==========================================
- Coverage   92.79%   92.78%   -0.01%     
==========================================
  Files        2374     2375       +1     
  Lines       76818    76888      +70     
==========================================
+ Hits        71280    71341      +61     
- Misses       5538     5547       +9

Impacted Files	Coverage Δ
test/workers/delete_plain_object_worker_test.rb	`100% <100%> (ø)`	⬆️
test/unit/apicast/proxy_source_test.rb	`100% <100%> (ø)`
app/models/proxy_rule.rb	`98.3% <100%> (+0.05%)`	⬆️
test/integration/api/integration_test.rb	`100% <100%> (ø)`	⬆️
app/controllers/api/integrations_controller.rb	`75% <90.9%> (+0.78%)`	⬆️
...tegration/api/application_plans_controller_test.rb	`88.88% <0%> (-11.12%)`	⬇️
app/models/payment_transaction.rb	`81.72% <0%> (+1.07%)`	⬆️

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 184f570...21e6d5a. Read the comment docs.

hallelujah

Ok so now we need to save the order in the db
Meaning we need to have a column for the position.
I think we are already doing that for some models.

didierofrivia · 2019-01-25T07:13:55Z

@hallelujah Only thing needed are what you mentioned.

hallelujah · 2019-02-26T15:32:31Z

app/views/api/integrations/apicast/shared/_mapping_rule.html.erb

@@ -2,6 +2,7 @@

  disabled ||= !rule.new_record? && !@marked_for_update.try(:include?,rule.id.to_s)
  html_options = { :disabled => disabled , :class => 'message-when-change' }
+  index ||= false


Do we need that?

Same template is used here https://github.com/3scale/porta/pull/530/files#diff-2eeed0d10ff9fe7aedaf9123ec70b297L25 where we are not passing index. So when it tries to access it here: https://github.com/3scale/porta/pull/530/files#diff-8aca4cca05f925b4b00a70b406f9638dR33, it breaks. Do you think it can be better some other way? (consider this integration page is not worthy a major refactor)

OK then you could use index = local_assigns[:index] instead of implicit variable assignment. And that will ensure that the variable exists whatever it is. variable ||= boolean is kinda ugly to read :)

indeed hehe, didn't know how to do something like that in ruby basically. thanks!

hallelujah · 2019-02-26T15:33:41Z

app/views/api/integrations/apicast/shared/_mapping_rules.html.slim

-          tr
-            = render 'api/integrations/apicast/shared/mapping_rule', form: f, rule: rule, metrics: metrics
-        tr#new-proxy-rule-template style="display:none;"
+        - @proxy.proxy_rules.ordered.each_with_index do |rule, i|


Maybe an instance variable in the controller or better a presenter instead of @proxy.proxy_rules.ordered

I'm not convinced this deserves a presenter given the short life expectancy of this integration page. Any other way you might suggest to change this?

hallelujah · 2019-02-26T15:38:47Z

app/views/api/integrations/apicast/shared/_mapping_rules.html.slim

    jQuery(this).val(jQuery(this).data('selected'))
  })
+  jQuery(function () {


Is this really needed? As I understand, on drag (start) it adds 4px width on each td where the element was dragged.

it happens here, it's more than 4px: https://github.com/3scale/porta/pull/530/files#diff-2eeed0d10ff9fe7aedaf9123ec70b297R39

If you do not add this code, will it be ugly? Can it be done in pure CSS?

Sadly nope, this trick is needed because of jqueryui#sortable lib and the use of tables.

hallelujah · 2019-02-26T15:39:36Z

db/migrate/20190125090030_add_position_to_mapping_rules.rb

@@ -0,0 +1,5 @@
+class AddPositionToMappingRules < ActiveRecord::Migration
+  def change
+    add_column :proxy_rules, :position, :integer


Does not need a migration to be done from @3scale/operations

hallelujah · 2019-02-26T15:41:29Z

db/migrate/20190221101857_add_last_to_proxy_rules.rb

@@ -0,0 +1,5 @@
+class AddLastToProxyRules < ActiveRecord::Migration
+  def change
+    add_column :proxy_rules, :last, :boolean, default: false


This needs a migration to be done by @3scale/operations because it has a NON-NULL default

@macejmic do not forget to remove that when this is merged #658

hallelujah · 2019-02-26T15:42:51Z

db/schema.rb

@@ -11,7 +11,7 @@
 #


You need to make the schema for Postgres and Oracle as well. make schema for Oracle IIRC

hallelujah · 2019-02-26T15:43:25Z

features/old/proxy.feature

    When I have policies feature enabled
    And apicast registry is stubbed
    And I go to the integration page for service "one"
    Then I should see the Policy Chain
+
+  @javascript @selenium


Now @selenium and @javascript are the same
They all both run under chrome-headless

hallelujah · 2019-02-26T15:45:40Z

features/step_definitions/proxy_steps.rb

+  end
+end
+
+Given(/^I drag the last mapping rule to the first position$/) do


Probably good to have it catch first position as (position \d+)

hallelujah

LGTM, would be nice to have the migration separated from this PR

It will be safe because it is adding a nullable column with a default

* Done the easiest way * This is meant to remove in the near future favoring an entire page craft in React using APIcast json schema config. * Missing some styling

hallelujah · 2019-03-01T15:36:42Z

app/controllers/api/integrations_controller.rb

@@ -201,6 +203,17 @@ def authorize
    authorize! :edit, @service
  end

+  def update_mapping_rules_position
+    proxy_rules_attributes.each_value do |attrs|
+      proxy_rule = @proxy.proxy_rules.find_by(id: attrs['id'])


that could be to fix codeclimate issue

Suggested change

proxy_rule = @proxy.proxy_rules.find_by(id: attrs['id'])

proxy_rule = @proxy.proxy_rules.find_by(id: attrs['id']) || next

hallelujah · 2019-03-01T15:37:17Z

app/controllers/api/integrations_controller.rb

+  def update_mapping_rules_position
+    proxy_rules_attributes.each_value do |attrs|
+      proxy_rule = @proxy.proxy_rules.find_by(id: attrs['id'])
+      proxy_rule&.set_list_position(attrs['position'])


Then remove the nil check

Suggested change

proxy_rule&.set_list_position(attrs['position'])

proxy_rule.set_list_position(attrs['position'])

didierofrivia self-assigned this Jan 22, 2019

hallelujah reviewed Jan 23, 2019

View reviewed changes

hallelujah self-assigned this Jan 24, 2019

didierofrivia force-pushed the feature/sorting-mapping-rules branch from 480de38 to 69b3c51 Compare January 25, 2019 07:12

didierofrivia force-pushed the feature/sorting-mapping-rules branch 2 times, most recently from eeb5030 to 3c8ffe0 Compare February 11, 2019 16:01

didierofrivia force-pushed the feature/sorting-mapping-rules branch from 679a3e8 to 50634d6 Compare February 25, 2019 11:26

macejmic temporarily deployed to preview01 February 25, 2019 14:29 Inactive

didierofrivia changed the title ~~[WIP] [Mapping Rules] Sorting mapping rules~~ [Mapping Rules] Sorting mapping rules Feb 26, 2019

didierofrivia temporarily deployed to preview01 February 26, 2019 15:20 Inactive

didierofrivia requested a review from a team February 26, 2019 15:23

hallelujah reviewed Feb 26, 2019

View reviewed changes

didierofrivia assigned macejmic Feb 27, 2019

macejmic force-pushed the feature/sorting-mapping-rules branch from c656377 to 07f8b04 Compare February 28, 2019 15:49

hallelujah previously approved these changes Feb 28, 2019

View reviewed changes

macejmic dismissed hallelujah’s stale review via ad34c52 March 1, 2019 11:15

macejmic force-pushed the feature/sorting-mapping-rules branch 2 times, most recently from 2ff2c02 to 94d522c Compare March 1, 2019 11:22

macejmic mentioned this pull request Mar 1, 2019

[1787] proxy rules new attributes last, position #658

Merged

didierofrivia added 2 commits March 1, 2019 12:54

[mapping rules] Sorting mapping rules

763be2a

* Done the easiest way * This is meant to remove in the near future favoring an entire page craft in React using APIcast json schema config. * Missing some styling

[mapping rules] Fix cell resizing when dragging

b14e67b

didierofrivia and others added 18 commits March 1, 2019 12:54

[mapping rules] Fix cell resizing when dragging

e2b5a06

[mapping rule] Position as a hidden field

449e5bf

[integration] save mapping rules position

d99daff

[gemfile] acts_as_list latest version

9497a96

[IntegrationsTest] test update positions

83cbdd7

[gemfile] update production gemfile

ffcd929

[gemfile] update acts_as_list to 0.9.0

0ede409

[gemfile] update acts_as_list to 0.9.17 and fix tests

5f6c6a1

final changes

013f838

[make bundle] fix all gemfiles

b396a44

[integrations controller] fix codeclimate issues

5c61d12

[mapping_rules] Last called checkbox added

53d2cee

[mapping_rules] typo

ab2591c

[mapping_rules] New order for new actions

fbe6a0c

[mapping_rules] Fix test and new mapping rule generation

c137f1b

[cuke] A more flexible feature

7579a17

[refactor] local assignment instead of ugly boolean

9745414

[new attributes] configuration + tests

7daf54f

macejmic force-pushed the feature/sorting-mapping-rules branch 2 times, most recently from 4f16739 to bd2c29a Compare March 1, 2019 12:37

hallelujah previously approved these changes Mar 1, 2019

View reviewed changes

hallelujah reviewed Mar 1, 2019

View reviewed changes

[integrations controller] codeclimate issues

21e6d5a

macejmic dismissed hallelujah’s stale review via 21e6d5a March 1, 2019 16:11

macejmic force-pushed the feature/sorting-mapping-rules branch from bd2c29a to 21e6d5a Compare March 1, 2019 16:11

macejmic merged commit 7dda0c8 into master Mar 1, 2019

macejmic deleted the feature/sorting-mapping-rules branch March 1, 2019 16:31

Martouta mentioned this pull request Mar 4, 2019

[Bugfix][Cuke] Check metric of sorted mapping rules by Name instead of ID #677

Merged

Martouta mentioned this pull request Dec 7, 2020

Remove rolling update independent_mapping_rules #2274

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mapping Rules] Sorting mapping rules #530

[Mapping Rules] Sorting mapping rules #530

didierofrivia commented Jan 22, 2019 •

edited by hallelujah

codecov bot commented Jan 23, 2019 •

edited

hallelujah left a comment

didierofrivia commented Jan 25, 2019

hallelujah Feb 26, 2019

didierofrivia Feb 26, 2019

hallelujah Feb 26, 2019 •

edited

didierofrivia Feb 26, 2019

hallelujah Feb 26, 2019 •

edited

didierofrivia Feb 26, 2019

hallelujah Feb 26, 2019

hallelujah Feb 26, 2019

didierofrivia Feb 26, 2019

hallelujah Feb 26, 2019

didierofrivia Feb 26, 2019

hallelujah Feb 26, 2019 •

edited

hallelujah Feb 26, 2019

hallelujah Mar 1, 2019

hallelujah Feb 26, 2019

hallelujah Feb 26, 2019 •

edited

hallelujah Feb 26, 2019 •

edited

hallelujah left a comment

hallelujah Mar 1, 2019 •

edited

hallelujah Mar 1, 2019

	proxy_rule = @proxy.proxy_rules.find_by(id: attrs['id'])
	proxy_rule = @proxy.proxy_rules.find_by(id: attrs['id']) \|\| next

	proxy_rule&.set_list_position(attrs['position'])
	proxy_rule.set_list_position(attrs['position'])

[Mapping Rules] Sorting mapping rules #530

[Mapping Rules] Sorting mapping rules #530

Conversation

didierofrivia commented Jan 22, 2019 • edited by hallelujah

codecov bot commented Jan 23, 2019 • edited

Codecov Report

hallelujah left a comment

Choose a reason for hiding this comment

didierofrivia commented Jan 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallelujah Feb 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hallelujah Feb 26, 2019 • edited

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

hallelujah Feb 26, 2019 • edited

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 Feb 26, 2019 • edited

Choose a reason for hiding this comment

hallelujah Feb 26, 2019 • edited

Choose a reason for hiding this comment

hallelujah left a comment

Choose a reason for hiding this comment

hallelujah Mar 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

didierofrivia commented Jan 22, 2019 •

edited by hallelujah

codecov bot commented Jan 23, 2019 •

edited

hallelujah Feb 26, 2019 •

edited

hallelujah Feb 26, 2019 •

edited

hallelujah Feb 26, 2019 •

edited

hallelujah Feb 26, 2019 •

edited

hallelujah Feb 26, 2019 •

edited

hallelujah Mar 1, 2019 •

edited