PR - Issue 50545 - Add the new replication monitor functionality to UI

[389-ds-bot]

Cloned from Pagure Pull-Request: https://pagure.io/389-ds-base/pull-request/50733

• Created at 2019-11-21 18:18:56 by spichugi (@droideck)
• Merged at 2020-01-13 19:51:18

---

Description: As we ported repl-monitor.pl to dscon CLI
we should add the functionality to WebUI.
It is important to keep in mind that we shouldn't expose
user's password so the interactive option should be carried out.

Improve replication monitor CLI JSON output consistency.
Add Full Replication report functionality with ability of
continuous refresh.

Resolves: #3601

Reviewed by: ?

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-11-22 19:46:11

console crashes, if I just click on generate report and enter binds credentials.

index.js:159957 Uncaught TypeError: report.items[Symbol.iterator] is not a function
    at Function.<anonymous> (index.js:159882)
    at s (cockpit.js:962)
    at cockpit.js:974
    at n (cockpit.js:880)

If I add credential, it still crashes

replMonitor.jsx:998 Uncaught TypeError: report.items[Symbol.iterator] is not a function
    at Function.<anonymous> (replMonitor.jsx:998)
    at s (cockpit.js:962)
    at cockpit.js:974
    at n (cockpit.js:880)
(anonymous)	@	replMonitor.jsx:998

Clicking on Add Alias also crashes browser:

checkPropTypes.js:19 Warning: Failed prop type: Invalid prop `port` of type `string` supplied to `ReportAliasesModal`, expected `number`.
    in ReportAliasesModal (created by ReplMonitor)
    in ReplMonitor (created by Monitor)
    in div (created by Monitor)
    in div (created by Monitor)
    in div (created by Monitor)
    in div (created by Monitor)
    in div (created by Monitor)
    in div (created by Monitor)
    in Monitor

Besides that I have some UX comments. I think the description paragraph is lacking details. I was not sure how to use it. Do I need to add "instance credentials" for each server I want to look at? It looks like if you don't have any defined it will look at the local agreements, but this is not described anywhere. You should say that you need to define all the replicas you want in the report, that you can pre-set the password or have it asked interactively. Explain what the alias table is, etc. Like I said I had no idea how to use it, so I just started pressing buttons, but if we are going have descriptive text then it should be comprehensive.

I think you should call it Replica Credentials Table and not Instance Credentials.

I think it would be useful to auto populate the table using the existing agreements, then they can be edited to add bind credentials. This will save an Admin a lot of time to setup and get a report. Add a column to the table stating if credentials are set for that replica.

Also, while you're at it. I think we should default all replica bind DN's to "cn=replication manager,cn=config". We do it some places but not all, and I think we should do it everywhere. In your report modals, but also in agreement creation modal, and anywhere else we ask for it.

Also what you did sort of obsoletes the "Lag Report" under the Replication Agreements tab. So maybe you can get rid of the Lag Report and its modals?

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-11-22 20:05:29

Also I think we should rename some of the tabs in replication monitor page. They are taking up a lot of horizontal space and it easily wraps on smaller monitors.

Full Topology Report --> Replication Report/Sync Report?
Replication Agreements -- > Agreements
Winsync Agreements --> Winsync

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-11-25 16:56:05

Also, while you're at it. I think we should default all replica bind DN's to "cn=replication manager,cn=config". We do it some places but not all, and I think we should do it everywhere. In your report modals, but also in agreement creation modal, and anywhere else we ask for it.

Actually in your authentication modals, it should default to "cn=directory manager" not "cn=replicaiton manager,cn=config", so you can ignore this for now...

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-11-25 18:59:20

Here is the code fix to get the report working and aliases:

diff --git a/src/cockpit/389-console/src/lib/monitor/replMonitor.jsx b/src/cockpit/389-console/src/lib/monitor/replMonitor.jsx
index 7fcdb3274..901ff797f 100644
--- a/src/cockpit/389-console/src/lib/monitor/replMonitor.jsx
+++ b/src/cockpit/389-console/src/lib/monitor/replMonitor.jsx
@@ -125,7 +125,7 @@ export class ReplMonitor extends React.Component {
             pwInputInterractive: false,
 
             aliasHostname: "",
-            aliasPort: "",
+            aliasPort: 389,
             aliasName: "",
 
             credentialsList: [],
@@ -903,7 +903,7 @@ export class ReplMonitor extends React.Component {
         this.setState({
             newEntry: true,
             aliasHostname: "",
-            aliasPort: "",
+            aliasPort: 389,
             aliasName: ""
         });
     }
@@ -913,7 +913,7 @@ export class ReplMonitor extends React.Component {
         this.setState({
             newEntry: false,
             aliasHostname: rowData.connData.split(':')[0],
-            aliasPort: rowData.connData.split(':')[1],
+            aliasPort: parseInt(rowData.connData.split(':')[1]),
             aliasName: rowData.alias
         });
     }
@@ -1001,11 +1001,10 @@ export class ReplMonitor extends React.Component {
                             agmts_reparsed = [];
                             if (replica.hasOwnProperty("agmts_status")) {
                                 for (let agmt of replica.agmts_status) {
-                                    let agmt_parsed = JSON.parse(agmt);
                                     // We need this for Agreement View Modal
-                                    agmt_parsed["supplierName"] = supplier.name;
-                                    agmt_parsed["replicaName"] = `${replica.replica_root}:${replica.replica_id}`;
-                                    agmts_reparsed.push(agmt_parsed);
+                                    agmt["supplierName"] = supplier.name;
+                                    agmt["replicaName"] = `${replica.replica_root}:${replica.replica_id}`;
+                                    agmts_reparsed.push(agmt);
                                 }
                             }
                             replica_reparsed.push({...replica, agmts_status: agmts_reparsed});
diff --git a/src/lib389/lib389/replica.py b/src/lib389/lib389/replica.py
index dfb08edd8..90df1cb77 100644
--- a/src/lib389/lib389/replica.py
+++ b/src/lib389/lib389/replica.py
@@ -2428,6 +2428,10 @@ class ReplicationMonitor(object):
                 if consumer not in report_data:
                     report_data[f"{consumer}:{protocol}"] = None
                 agmts_status.append(agmt.status(use_json=use_json))
+                if use_json:
+                    agmts_status.append(json.loads(agmt.status(use_json=True)))
+                else:
+                    agmts_status.append(agmt.status())
             replicas_status.append({"replica_id": replica_id,
                                     "replica_root": replica_root,
                                     "maxcsn": replica_maxcsn,

Next, a few issues I noticed

[1] When generating the report there is a long lag between prompting for credentials, or just generating the report. A spinner should be used, or something, to show that the UI is doing something during then entire process

[2] Generating a report, I get prompted for the local credentials, why? These credentials are already available

[3] the report modal. The "Close Report' button should be a primary button not a default button

[4] The modal report is cramped and narrow. What happens when you have 4 masters and 10 agreements? Is it just one long report? I think I created a "wide modal" css, I think you should use it, and it will make the report slightly more readable. I wonder we should have an entire page (not a modal) dedicated to the report? Maybe do a tabbed pane (one settings, the other for the report)? It's such a useful report, it would be a shame to not present it in the most readable/useful way.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-11-26 08:12:54

console crashes, if I just click on generate report and enter binds credentials.
If I add credential, it still crashes

For some reason, both issues are not present for me... I can add, edit and remove credentials without any crashes.

Clicking on Add Alias also crashes browser:

Missed that one! The report still displays an alias correctly even after the error. But it's fixed, thanks!

Besides that I have some UX comments. I think the description paragraph is lacking details. I was not sure how to use it. Do I need to add "instance credentials" for each server I want to look at?

It has a title(hint) on both hostname and port that it accepts regular expressions. But I think I will add a small How to` hidable note in the report preparation tab.

I think you should call it Replica Credentials Table and not Instance Credentials.

Sure, sounds good.

I think it would be useful to auto populate the table using the existing agreements, then they can be edited to add bind credentials. This will save an Admin a lot of time to setup and get a report. Add a column to the table stating if credentials are set for that replica.

Good catch!

Also what you did sort of obsoletes the "Lag Report" under the Replication Agreements tab. So maybe you can get rid of the Lag Report and its modals?

Yeah, I had the same thought. You can initialize the agreement from Lag Report modal but it's a bonus, I think. I will get rid of Lag Report button.

Here is the code fix to get the report working and aliases:

Thanks! Very helpful!

Next, a few issues I noticed
[1] When generating the report there is a long lag between prompting for credentials, or just generating the report. A spinner should be used, or something, to show that the UI is doing something during then entire process

Yeah, I even have the state for that but apparently it wasn't connected to any element. I'll add it.

[2] Generating a report, I get prompted for the local credentials, why? These credentials are already available

It is how we process the instances. We check the agreement credentials and the protocol type and connect using this way.
For example, you may be connected to the instance through 389 port but the agreements work through 636 (and maybe some other hostname). I think we shouldn't ignore that and we should open the exact connection that is defined in the agreement.

[3] the report modal. The "Close Report' button should be a primary button not a default button

It will look better, true!

[4] The modal report is cramped and narrow. What happens when you have 4 masters and 10 agreements? Is it just one long report? I think I created a "wide modal" css, I think you should use it, and it will make the report slightly more readable. I wonder we should have an entire page (not a modal) dedicated to the report? Maybe do a tabbed pane (one settings, the other for the report)? It's such a useful report, it would be a shame to not present it in the most readable/useful way.

I had the same feeling. I've tried to use 'wide' but it wasn't working properly (and in general I don't like to diverse far from PF4 CSS because it may create the issues in the future).
But I like the tab idea a lot. So I think I can add two tabs under Sync Report tab - one is Preparation and another is Result (I'll think a bit more about exact words...).
It will make everything look better for sure.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-03 15:25:07

rebased onto 2b8d25abad60c1427835dd4900adc6449f7357ed

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-03 15:25:22

Fixed. Please, review.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-05 19:24:46

This is a bug on my part from a previous patch, but you should add it:

monitor.jsx line 1175

              if (this.state.replicatedSuffixes.length < 1) {
                    monitor_element =
                        <div>
                            <p>There are no suffixes that have been configured for replication</p>
                        </div>;
                    // Must enable tree since we are not calling any components
                    this.enableTree();

---> Must enableTree here in this case

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-05 19:37:33

In the replica/credential table can you change this text to use italics: Edit To Add a Bind DN Data

There should be a table header to say what the "credential table" is.

The Bind DN in the report modals do not appear to be auto filled with "cn=directory manager"

There should be a success notification once the report is written, and it should state that you need to goto the report tab. Or better yet, move the UI to the report tab once the report is written.

Finally on the report tab, the Refresh and Refresh timeout header is somewhat distracting. Maybe remove the bold or even shrink the font size. It just needs better separation from the report. It's kind of blending in, and taking away from the "report" IMHO

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-05 19:45:31

This is a bug on my part from a previous patch, but you should add it:
monitor.jsx line 1175
if (this.state.replicatedSuffixes.length < 1) {
monitor_element =

There are no suffixes that have been configured for replication

;
// Must enable tree since we are not calling any components
this.enableTree();

---> Must enableTree here in this case

This code won't work. I think we need to call the replMonitor component with a null suffix, and in the component we check for the null suffix and just enable the tree, and return the "There are no suffixes that have been configured for replication" paragraph. Otherwise the entire monitor treeview is disabled after clicking the replication node (if there are no replicated suffixes).

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-06 15:18:08

rebased onto cbd42bf5fa5f05633f1e29751c63e390a349da82

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-06 15:27:34

In the replica/credential table can you change this text to use italics: Edit To Add a Bind DN Data

Fixed.

There should be a table header to say what the "credential table" is.

I did try to add it and I think it looks redundant.
We already have the header when there is not data in the table (as we have for every other table in our WebUI). Also, we have a button Add Credentials which makes it clear what's in the table (and we see that there are credentials in the table itself)

The Bind DN in the report modals do not appear to be auto filled with "cn=directory manager"

Done.

There should be a success notification once the report is written, and it should state that you need to goto the report tab. Or better yet, move the UI to the report tab once the report is written.

I already have the feature.
There was a bug that the tab was not switched to Report when we input the credentials interactively.

Finally on the report tab, the Refresh and Refresh timeout header is somewhat distracting. Maybe remove the bold or even shrink the font size. It just needs better separation from the report. It's kind of blending in, and taking away from the "report" IMHO

Done.

Also, I've fixed the issue you have reported. I've just added

} else {
        // We should enable it here because ReplMonitor never will be mounted
        this.enableTree();
}

to loadMonitorReplication(). So when we don't run the loading chain - we just enable the tree.

And there was one crash that is fixed now. When we load Replication Suffix data and we try to select Database Suffix tab - it crashes because dc=example,dc=com is found and the database suffix data is not loaded. I've extended if:

if (
            selectedNode.id in this.state &&
            ("chainingData" in this.state[selectedNode.id] ||
                "suffixData" in this.state[selectedNode.id])
) {
            // This suffix is already cached

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-06 15:33:03

3 new commits added

• Fix issues 2
• Fix various issues
• Issue 50545 - Add the new replication monitor functionality to UI

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-06 21:10:10

It looks a lot better but the report seems off. I'm attaching a screenshot to the Issue (since I can't do it in the PR)

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-06 22:01:18

It looks a lot better but the report seems off. I'm attaching a screenshot to the Issue (since I can't do it in the PR)

Could you please send me an email with your agreements and replica entries?

For me, the report prints clearly because my hostnames are the same everywhere.. So I wonder how exactly reproduce what you have

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-06 22:06:31

It looks a lot better but the report seems off. I'm attaching a screenshot to the Issue (since I can't do it in the PR)

Could you please send me an email with your agreements and replica entries?
For me, the report prints clearly because my hostnames are the same everywhere.. So I wonder how exactly reproduce what you have

Here is the config and agreement from the instance I am generating the report . The other replica does not have any agreements:

dn: cn=replica,cn=dc\3Dexample\2Cdc\3Dcom,cn=mapping tree,cn=config
objectClass: top
objectClass: nsds5Replica
cn: replica
nsDS5ReplicaRoot: dc=example,dc=com
nsDS5Flags: 1
nsDS5ReplicaType: 3
nsDS5ReplicaId: 2
nsDS5ReplicaBindDN: cn=replication manager,cn=config
nsState:: AgAAAAAAAAAUSuldAAAAAAAAAAAAAAAAAAAAAAAAAAABAAAAAAAAAA==
nsDS5ReplicaName: 91aabc03-178b11ea-a2c4bc99-2f841849


dn: cn=master,cn=replica,cn=dc\3Dexample\2Cdc\3Dcom,cn=mapping tree,cn=config
objectClass: top
objectClass: nsds5replicationagreement
cn: master
nsDS5ReplicaRoot: dc=example,dc=com
description: master
nsDS5ReplicaHost: localhost
nsDS5ReplicaPort: 389
nsDS5ReplicaBindMethod: simple
nsDS5ReplicaTransportInfo: LDAP
nsDS5ReplicaBindDN: cn=replication manager,cn=config
nsDS5ReplicaCredentials: ...
nsds50ruv: {replicageneration} 5de71816000000010000
nsds50ruv: {replica 1 ldap://localhost.localdomain:389}
nsruvReplicaLastModified: {replica 1 ldap://localhost.localdomain:389} 00000000

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-06 22:08:57

For completeness this is replica entry from the other master (that has no agreements)

dn: cn=replica,cn=dc\3Dexample\2Cdc\3Dcom,cn=mapping tree,cn=config
objectClass: top
objectClass: nsds5Replica
cn: replica
nsDS5ReplicaRoot: dc=example,dc=com
nsDS5Flags: 1
nsDS5ReplicaType: 3
nsDS5ReplicaId: 1
nsDS5ReplicaBindDN: cn=replication manager,cn=config
nsState:: AQAAAAAAAADPtOpdAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA==
nsDS5ReplicaName: d2760403-186311ea-a803c30d-f0de2b88

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-07 01:03:32

1 new commit added

• Filter repeated items and get the port correctly

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-07 01:04:18

Okay, I found the cause of the issues. Please, check.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-07 18:10:50

Much better, but I have have some concerns. I updated the ticket with screen shots and comments.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-11 00:25:07

1 new commit added

• Fix eslintrc indent conflict

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-11 00:27:52

More improvements! Please, check.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-16 17:35:03

After clicking "generate report" the Auth Modal "Bind DN" field is disabled and I can not change it.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-16 17:42:45

If I edit the connection in the Cred Table the new bind DN sticks, but it should be editable in the modal. I like how you have a switch to show the table, but I don't like the title "All-In-One". Maybe call it "Table View"?

Minor issues: the "Refresh" label and checkbox are really far apart. They should be in the same Column, not in separate columns, same for the Timeout field and label - they should be connected/static.

Besides that I think we are good to go! Thanks for listening to all my annoying suggestions.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-16 18:09:43

One more thing :-)

In the Report Table, the table itself is really wide. I think we can remove the "Is Enabled" column. For this report we should not be looking at disabled agreements anyway.

That is all...

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-19 17:06:31

rebased onto 3b10d22a52db28aa121b53468c7bac93c8b489ad

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-19 17:08:30

Changes are made...

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2019-12-19 18:37:42

Now I am not prompted at all when generating the report, and it still uses "cn=directory manager" not my custom Root DN value (cn=dm). Then the browser crashes and dsconf did not report a useful error message:

replMonitor.jsx:1028 MARK err:  B {problem: null, exit_status: 1, exit_signal: null, message: "dsconf exited with code 1", toString: ƒ}exit_signal: nullexit_status: 1message: "dsconf exited with code 1"problem: nulltoString: ƒ ()__proto__: Object

12:24:09.453 VM1301:1 Uncaught SyntaxError: Unexpected token d in JSON at position 0
    at JSON.parse (<anonymous>)
    at Function.<anonymous> (replMonitor.jsx:1029)
    at s (cockpit.js:962)
    at cockpit.js:974
    at n (cockpit.js:880)
(anonymous) @ replMonitor.jsx:1029

Even if I edit the connection to use the correct bind DN the browser still crashes the same way.

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-19 23:35:18

1 new commit added

• Correct BindDN auto-fill

[389-ds-bot]

Comment from spichugi (@droideck) at 2019-12-20 01:35:09

7 new commits added

• Correct BindDN auto-fill
• Make more improvements
• Fix eslintrc indent conflict
• Filter repeated items and get the port correctly
• Fix issues 2
• Fix various issues
• Issue 50545 - Add the new replication monitor functionality to UI

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-10 17:02:42

rebased onto 4ec9e6744ee697a35191304569e627da4c6fecf8

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-10 17:03:32

Fixed a small issue with clearing dynamic credentials at a failure.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-01-10 19:29:22

When I goto the replication monitor and click generate report (without doing anything else ) I do NOT get prompted for a password (even though it says interactive prompt is set), and I get an error:

Sync report has failed - Please, fill in all Credential details. It should be host:port:binddn:bindpw

So for the connection to the remote replica it does not have a bind DN set. If I add a bind DN for the other connection then when I click on generate report it does propt for the password, but it should prompt when the bind DN is also not set. Ir better yet, if you don't know the remote bind DN then use the local bind DN. Typically people use the same Root DN for all their servers, so it's a good assumption to make

After entering all the correct data, the report works, and I do not see any console crashes.

However the header of the report, that has all the options, is still bulky. The font size should be reduced, and I think the table columns should be wider. Also the text "loading data" is word wrapped, no need for that. If you don't mind please consider applying this patch to your PR

diff --git a/src/cockpit/389-console/src/css/ds.css b/src/cockpit/389-console/src/css/ds.css
index e6bb1da5d..662d8de5f 100644
--- a/src/cockpit/389-console/src/css/ds.css
+++ b/src/cockpit/389-console/src/css/ds.css
@@ -885,6 +885,10 @@ option {
     padding-left: 5px;
 }
 
+.ds-raise-field {
+    margin-top: -3px;
+}
+
 .content-view-pf-pagination > div > span:last-child {
     position: relative;
 }
diff --git a/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx b/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx
index aef07f809..f7a46b1a2 100644
--- a/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx
+++ b/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx
@@ -1,5 +1,4 @@
 import React from "react";
-import Switch from "react-switch";
 import {
     Modal,
     Row,
@@ -849,7 +848,7 @@ class FullReportContent extends React.Component {
         let spinner = "";
         if (reportLoading) {
             spinner = (
-                <Col sm={3}>
+                <Col sm={12} className="ds-center ds-margin-top">
                     <ControlLabel title="Do the refresh every few seconds">
                         {reportRefreshing ? "Refreshing" : "Loading"} the report...
                     </ControlLabel>
@@ -862,54 +861,60 @@ class FullReportContent extends React.Component {
             reportHeader = (
                 <Form horizontal autoComplete="off">
                     <FormGroup controlId="reportRefreshTimeout">
-                        <Col sm={3}>
-                            <Checkbox
-                                checked={reportRefreshing}
-                                id="reportRefreshing"
-                                onChange={handleRefresh}
-                                title="Do the refresh every few seconds"
-                            >
-                                Refresh
-                            </Checkbox>
-                        </Col>
-                        <Col componentClass={ControlLabel} sm={2} title="Refresh Timeout">
-                            Timeout (seconds)
-                        </Col>
-                        <Col sm={2}>
-                            <FormControl
-                                id="reportRefreshTimeout"
-                                type="number"
-                                min="1"
-                                max="65534"
-                                value={reportRefreshTimeout}
-                                onChange={handleFieldChange}
-                            />
+                        <Col sm={5}>
+                            <div className="ds-inline">
+                                <Checkbox
+                                    checked={reportRefreshing}
+                                    id="reportRefreshing"
+                                    onChange={handleRefresh}
+                                    title="Do the refresh every few seconds"
+                                >
+                                    Refresh
+                                </Checkbox>
+                            </div>
+                            <div className="ds-inline ds-left-margin ds-raise-field">
+                                <FormControl
+                                    type="number"
+                                    min="1"
+                                    max="65534"
+                                    className="ds-input-right"
+                                    value={reportRefreshTimeout}
+                                    onChange={handleFieldChange}
+                                    title="Refresh timeout in seconds"
+                                />
+                            </div>
+                            <div className="ds-inline ds-left-margin">
+                                <font size="2">seconds</font>
+                            </div>
                         </Col>
-                        {spinner}
                     </FormGroup>
-                    <FormGroup controlId="oneTableReport">
-                        <Col sm={3}>
+                    <FormGroup controlId="showDisabledAgreements">
+                        <Col sm={8}>
                             <Checkbox
                                 checked={this.state.showDisabledAgreements}
                                 id="showDisabledAgreements"
                                 onChange={this.handleSwitchChange}
-                                title="Display all agreements including the disabled and the ones that we are failed to connect to"
+                                title="Display all agreements including the disabled ones and the ones we failed to connect to"
                             >
                                 Show All (Including Disabled Agreements)
                             </Checkbox>
                         </Col>
-                        <Col componentClass={ControlLabel} sm={2} title="Show all data in one table (it makes it easier to check lag times)">
-                            Table View
-                        </Col>
-                        <Col sm={3}>
-                            <Switch
-                                className="ds-switch"
-                                onChange={this.handleSwitchChange}
+                    </FormGroup>
+                    <FormGroup controlId="oneTableReport">
+                        <Col sm={6} title="Show all data in one table (it makes it easier to check lag times)">
+                            <Checkbox
                                 checked={this.state.oneTableReport}
-                                height={20}
-                            />
+                                onChange={this.handleSwitchChange}
+                                id="oneTableReport"
+                                title="Display all agreements including the disabled ones and the ones we failed to connect to"
+                            >
+                                Table View
+                            </Checkbox>
                         </Col>
                     </FormGroup>
+                    <FormGroup>
+                        {spinner}
+                    </FormGroup>
                     <hr />
                 </Form>
             );

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-11 01:23:20

rebased onto aec00692c5d744de373253288661a1d4f3d96187

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-11 01:27:30

Thanks! I like how it looks.

I've just fixed one thing - when we set refresh timeout = 1, it will refresh the UI very often and the whole report will be bouncing each second.

So I've initialise the spinner variable with an empty ControlLablel so at least some element is always present there.

Please, check.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-01-13 15:21:44

Thanks Simon! I have one more patch to suggest. It sets the columns width for the refresh setting to the max, this prevents it from wrapping on smaller screens.

I set the minimum refresh to 5 seconds, With lots of replicas and agreements, one second refresh would never complete anyway. So I set it to 5 seconds as the minimum.\

Then, especially in PF4 being "bigger", vertical space on the page is limited. So I changed the refresh spinner to replace the report body when loading.

Let me know what you think. Feel free to tweak it anyway you want:

diff --git a/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx b/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx
index 952fbf047..ef4e83025 100644
--- a/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx
+++ b/src/cockpit/389-console/src/lib/monitor/monitorModals.jsx
@@ -861,7 +861,7 @@ class FullReportContent extends React.Component {
             reportHeader = (
                 <Form horizontal autoComplete="off">
                     <FormGroup controlId="reportRefreshTimeout">
-                        <Col sm={5}>
+                        <Col sm={12}>
                             <div className="ds-inline">
                                 <Checkbox
                                     checked={reportRefreshing}
@@ -875,7 +875,7 @@ class FullReportContent extends React.Component {
                             <div className="ds-inline ds-left-margin ds-raise-field">
                                 <FormControl
                                     type="number"
-                                    min="1"
+                                    min="5"
                                     max="65534"
                                     className="ds-input-right"
                                     value={reportRefreshTimeout}
@@ -912,11 +912,6 @@ class FullReportContent extends React.Component {
                             </Checkbox>
                         </Col>
                     </FormGroup>
-                    <FormGroup>
-                        <Col sm={12} className="ds-center ds-margin-top">
-                            {spinner}
-                        </Col>
-                    </FormGroup>
                     <hr />
                 </Form>
             );
@@ -996,15 +991,22 @@ class FullReportContent extends React.Component {
             }
         }
 
+        let report = suppliers.map(supplier => (
+            <div key={supplier.key}>
+                {supplier}
+                <hr />
+            </div>
+        ));
+        if (reportLoading) {
+            report =
+                <Col sm={12} className="ds-center ds-margin-top">
+                    {spinner}
+                </Col>;
+        }
         return (
             <div>
                 {reportHeader}
-                {suppliers.map(supplier => (
-                    <div key={supplier.key}>
-                        {supplier}
-                        <hr />
-                    </div>
-                ))}
+                {report}
             </div>
         );
     }

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-13 17:01:59

I like all of the changes except this one:

Then, especially in PF4 being "bigger", vertical space on the page is limited. So I changed the refresh spinner to replace the report body when loading.

I think it breaks the report usability a lot.
For example, you what to continuously refresh a big report on a slow machine. It will make the feature (continuous refresh) unusable because the report will be present on the screen only for 1 second and the rest of the time it will be Refreshing the report... And it will be like this all the time - 1-2 second of ther report, 3-4 seconds of the Refreshing...

Or what point of view on the continuous refresh feature do you have? Maybe I miss something

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-01-13 17:19:48

To be honest, i don't think continuous refresh is that useful for replication reports. It's just not that dynamic. If things are "off", it could take hours for them to get "fixed". I can't imagine someone wanting a refresh to be done every 5 or 10 seconds. Maybe it could just be a button, and refresh it when "you" want to?

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-13 19:28:00

1 new commit added

• Change continuous refresh to a button

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-13 19:29:49

Actually, yeah, sounds good! I think it makes sense.
Please, check.

[389-ds-bot]

Comment from mreynolds (@mreynolds389) at 2020-01-13 19:36:34

Looks good! ACK!

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-13 19:49:41

rebased onto 3054205

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-13 19:50:50

Thanks!

[389-ds-bot]

Comment from spichugi (@droideck) at 2020-01-13 19:51:18

Pull-Request has been merged by droideck

[389-ds-bot]

Patch
50733.patch