Replication excluded tables UI moved to the replication tab #12604

lgalis · 2016-11-14T06:55:43Z

Updating replication excluded tables UI moved to the replication tab

Links

This is the UI change for
https://bugzilla.redhat.com/show_bug.cgi?id=1389821

Depends on #12592

Steps for Testing/QA

carbonin · 2016-11-14T18:20:12Z

app/controllers/ops_controller/settings/common.rb

-        add_flash(_("Replication configuration save was successful"))
+        if replication_type == :remote && !params[:exclusion_list].empty?
+          begin
+            new_exclusion_list = params[:exclusion_list].split("\n- ").reject { |s| s.eql?('---') }


I don't think we should roll our own YAML parsing. I would rather use YAML.load and deal with the brakeman issues in some other way.

@carbonin - agree - changed to use safe_load

carbonin · 2016-11-14T18:24:10Z

When testing this change, it seems like the page is rendering the active_excludes which doesn't actually include all of the tables that should be excluded by the time the page comes up. This is because we are queueing the refresh of the excludes.

The result is that tables get removed from the excludes if you resubmit the excludes after making another change.

I'm not sure what way we should go with this. Maybe we don't queue the refresh or maybe we can do everything in some kind of transaction so that you don't see any excludes until the transaction finishes, but that will lead to people seeing an empty list of excludes ... not sure which is better. @Fryguy thoughts on this?

--- Edit:
I think this is addressed because we don't re-query the excludes when rendering the page after a save. Instead we display what was entered which doesn't rely on the current state of the process of changing the excludes in the background.

carbonin · 2016-11-14T19:32:37Z

This is what the page looks like after both changes:

carbonin · 2016-11-14T20:40:18Z

#12592 has been merged.

lgalis · 2016-11-14T22:01:21Z

@miq-bot remove_label wip

lgalis · 2016-11-14T22:02:58Z

@miq-bot add_label blocker

lgalis · 2016-11-14T23:06:39Z

/cc @carbonin @gtanzillo

carbonin · 2016-11-15T14:02:53Z

app/controllers/ops_controller/settings/common.rb

@@ -183,9 +183,16 @@ def pglogical_subscriptions_form_fields
      end
    end

+    exclusion_list = if replication_type == :remote


I think you can one-line this:
exclusion_list = replication_type == :remote ? MiqPglogical.new.active_excludes : MiqPglogical.default_excludes

carbonin · 2016-11-15T14:16:06Z

app/controllers/ops_controller/settings/common.rb

@@ -218,7 +225,18 @@ def pglogical_save_subscriptions
        add_flash(_("Error during replication configuration save: %{message}") %
                    {:message => bang.message}, :error)
      else
-        add_flash(_("Replication configuration save was successful"))
+        if replication_type == :remote && !params[:exclusion_list].empty?


I'm thinking this code should probably be in a separate method.

I don't think this really belongs in pglogical_save_subscriptions right?

I'm thinking the entry point should be a separate, more generic method. Then depending on the replication type we should call something like pglogical_save_subscriptions or pglogical_save_excludes. That would make this a lot easier to understand.

Then we could also remove some duplication of the error handling code and also of MiqRegion.replication_type = replication_type calls (which we seem to have two of).

@carbonin - That method is invoked by the save button - it could have had a different name, I agree, but it handles the save for the entire replication tab. I am not sure I want to separate it for this PR.

miq-bot · 2016-11-15T16:18:31Z

Checked commits lgalis/manageiq@865abd1~...8466ad4 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. 👍

carbonin · 2016-11-15T17:08:32Z

@lgalis can you assign to someone on the UI team for review? I'm not sure who should take this.

lgalis · 2016-11-15T17:44:17Z

@h-kataria - please review

lgalis · 2016-11-15T17:44:37Z

@miq-bot assign @h-kataria

h-kataria · 2016-11-16T15:27:36Z

Don't have a setup to test save of exclude list, overall changes look good.

…bles_ui Replication excluded tables UI moved to the replication tab (cherry picked from commit e28727a) https://bugzilla.redhat.com/show_bug.cgi?id=1389821

chessbyte · 2016-11-16T17:43:14Z

Euwe Backport details:

$ git log -1
commit 4fd04ca2f56d19f31921d5f75224da18b2e40c49
Author: Harpreet Kataria <hkataria@redhat.com>
Date:   Wed Nov 16 10:27:43 2016 -0500

    Merge pull request #12604 from lgalis/separate_replication_exclude_tables_ui

    Replication excluded tables UI moved to the replication tab
    (cherry picked from commit e28727a3cae37d561d8dd7daca87e94726cbddf0)

    https://bugzilla.redhat.com/show_bug.cgi?id=1389821

lgalis changed the title ~~[WIP] Updating replication excluded tables UI moved to the replication tab~~ [WIP]Replication excluded tables UI moved to the replication tab Nov 14, 2016

lgalis force-pushed the separate_replication_exclude_tables_ui branch from 1b35423 to e0d6bc1 Compare November 14, 2016 13:34

carbonin added bug core/replication ui wip labels Nov 14, 2016

lgalis force-pushed the separate_replication_exclude_tables_ui branch 2 times, most recently from faf92b1 to 545a4c0 Compare November 14, 2016 15:28

carbonin reviewed Nov 14, 2016

View reviewed changes

lgalis force-pushed the separate_replication_exclude_tables_ui branch 2 times, most recently from 137ab45 to d739610 Compare November 14, 2016 18:53

carbonin added the euwe/yes label Nov 14, 2016

lgalis added 2 commits November 14, 2016 16:31

Replication Exclusion List UI

865abd1

Added exclusion list to the specs and code style changes for controller

03d0237

chessbyte changed the title ~~[WIP]Replication excluded tables UI moved to the replication tab~~ [WIP] Replication excluded tables UI moved to the replication tab Nov 14, 2016

lgalis force-pushed the separate_replication_exclude_tables_ui branch from 70e6bfd to aef6cd2 Compare November 14, 2016 22:00

miq-bot removed the wip label Nov 14, 2016

lgalis changed the title ~~[WIP] Replication excluded tables UI moved to the replication tab~~ Replication excluded tables UI moved to the replication tab Nov 14, 2016

miq-bot added the blocker label Nov 14, 2016

Specs for replication excluded list

1771a61

lgalis force-pushed the separate_replication_exclude_tables_ui branch from aef6cd2 to 1771a61 Compare November 14, 2016 22:05

carbonin reviewed Nov 15, 2016

View reviewed changes

Replace if with ternary operator

23e87be

Add { for if/else

8466ad4

miq-bot assigned h-kataria Nov 15, 2016

lgalis closed this Nov 16, 2016

lgalis reopened this Nov 16, 2016

himdel closed this Nov 16, 2016

himdel reopened this Nov 16, 2016

himdel mentioned this pull request Nov 16, 2016

[WIP] Move frontend deps from bower to npm #12581

Closed

7 tasks

h-kataria added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 16, 2016

h-kataria merged commit e28727a into ManageIQ:master Nov 16, 2016

chessbyte added euwe/backported and removed euwe/yes labels Nov 16, 2016

lgalis deleted the separate_replication_exclude_tables_ui branch December 9, 2016 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replication excluded tables UI moved to the replication tab #12604

Replication excluded tables UI moved to the replication tab #12604

lgalis commented Nov 14, 2016 •

edited by miq-bot

Loading

carbonin Nov 14, 2016

lgalis Nov 14, 2016

carbonin commented Nov 14, 2016 •

edited

Loading

carbonin commented Nov 14, 2016

carbonin commented Nov 14, 2016

lgalis commented Nov 14, 2016

lgalis commented Nov 14, 2016

lgalis commented Nov 14, 2016

carbonin Nov 15, 2016

carbonin Nov 15, 2016

lgalis Nov 15, 2016 •

edited

Loading

miq-bot commented Nov 15, 2016

carbonin commented Nov 15, 2016

lgalis commented Nov 15, 2016

lgalis commented Nov 15, 2016

h-kataria commented Nov 16, 2016

chessbyte commented Nov 16, 2016

Replication excluded tables UI moved to the replication tab #12604

Replication excluded tables UI moved to the replication tab #12604

Conversation

lgalis commented Nov 14, 2016 • edited by miq-bot Loading

Links

carbonin Nov 14, 2016

Choose a reason for hiding this comment

lgalis Nov 14, 2016

Choose a reason for hiding this comment

carbonin commented Nov 14, 2016 • edited Loading

carbonin commented Nov 14, 2016

carbonin commented Nov 14, 2016

lgalis commented Nov 14, 2016

lgalis commented Nov 14, 2016

lgalis commented Nov 14, 2016

carbonin Nov 15, 2016

Choose a reason for hiding this comment

carbonin Nov 15, 2016

Choose a reason for hiding this comment

lgalis Nov 15, 2016 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Nov 15, 2016

carbonin commented Nov 15, 2016

lgalis commented Nov 15, 2016

lgalis commented Nov 15, 2016

h-kataria commented Nov 16, 2016

chessbyte commented Nov 16, 2016

lgalis commented Nov 14, 2016 •

edited by miq-bot

Loading

carbonin commented Nov 14, 2016 •

edited

Loading

lgalis Nov 15, 2016 •

edited

Loading