'var' makes the variable accessible in the whole function, which is better to avoid, it can create a mess.
'let' only accessible in the block it’s defined in.

This thing makes it much more error proof with let.

Same here. I'd say, replace all 'var' with 'let'.
Also, if 'var' defined outside of function - it becomes global which is really wrong in most-most cases...

Looks like the function only does elements[i].setAttribute('title', elements[i].innerText);... The rest is commented out. Can we remove the function and use the React state approach for updating the elements (if we need it at all)?

Not a bit issue but it may be more convenient to use a new 'for' loop format:

            for (let row of rows) {
                fullList.push(row.name);
            }

It can be 'const' because it wouldn't be changed during the single render.

Another thing, I think it is important to have the props definitions like this for all components you add:

MemberOf.propTypes = {
    rows: PropTypes.array,
    serverId: PropTypes.string,
    savePluginHandler: PropTypes.func,
    pluginListHandler: PropTypes.func,
    addNotification: PropTypes.func,
    toggleLoadingHandler: PropTypes.func
};
MemberOf.defaultProps = {
    rows: [],
    serverId: "",
    savePluginHandler: noop,
    pluginListHandler: noop,
    addNotification: noop,
    toggleLoadingHandler: noop
};

The reasons why it is must have:

• it is much easier to follow data flow in the application if you see what is passed and which are the defaults;
• it makes the code less likely to have errors with types;
• it becomes much easier to maintain the code in the future.

I'll test the tab manually later today and tomorrow but from what I see, all the issues are minor. The job is very well done!

Another thing, I think it is important to have the props definitions like this for all components you add:
MemberOf.propTypes = {
rows: PropTypes.array,
serverId: PropTypes.string,
savePluginHandler: PropTypes.func,
pluginListHandler: PropTypes.func,
addNotification: PropTypes.func,
toggleLoadingHandler: PropTypes.func
};

MemberOf.defaultProps = {
rows: [],
serverId: "",
savePluginHandler: noop,
pluginListHandler: noop,
addNotification: noop,
toggleLoadingHandler: noop
};

The reasons why it is must have:

it is much easier to follow data flow in the application if you see what is passed and which are the defaults;
it makes the code less likely to have errors with types;
it becomes much easier to maintain the code in the future.

Oh yeah I forgot to do this. Sorry the patch was so big I got lost in it. That's why I wanted to get it out for review before I went on PTO. I will address all of these issues (I'm sure you will find more) - thanks for reviewing so far!

This looks odd, why do we have a dict doing to a function in args? Something I missing in tasks?

I think there is an "ensure" type on ldapobjects that does this style wrapping? It's probably better not to confuse "create" which is an action, with "ensure" which is stateful.

There are a couple of errors in the console log:

[DOM] Found 2 elements with non-unique id #find: (More info: https://goo.gl/9p2vKq)
<input placeholder type="text" id="find" class=​"form-control" value>​
<input placeholder type=​"text" id=​"find" class=​"form-control" value>​
[DOM] Found 2 elements with non-unique id #pagination-row-dropdown: (More info: https://goo.gl/9p2vKq)
<button id=​"pagination-row-dropdown" role=​"button" aria-haspopup=​"true" aria-expanded=​"false" type=​"button" class=​"dropdown-toggle btn btn-default">​…​</button>​
<button id=​"pagination-row-dropdown" role=​"button" aria-haspopup=​"true" aria-expanded=​"false" type=​"button" class=​"dropdown-toggle btn btn-default">​…​</button>​

But I am not sure why the IDs are not unique... The elements are patternfly-react objects.

The data appears in the fields with a delay (Global Database Configuration and Chaining Configuration tabs). I think it makes sense to add the loader spinner at some visible point (check Plugins tab loader).
Ideally, it should appear on every action (like saving configuration etc.)

Delete chaining LDAP controls fails with errors in the console log:

CMD: deleteOids: Delete chaining controls ==> dsconf -j ldapi://%2fvar%2frun%2fslapd-local3.socket chaining config-set --del-control=1.2.840.113556.1.4.473
cockpit.js:585 'NoneType' object is not iterable
index.js:132782 Uncaught TypeError: _this6.loadGlobalConfig is not a function
at Function.<anonymous> (index.js:132782)
at s (cockpit.js:958)
at cockpit.js:970
at n (cockpit.js:876)

When we press 'Save' button on any of the tabs, we get the message that "Successfully updated something" even when we changed nothing in the interface.

I think it is not 'must have' but it'll be nice to inform the user that nothing has happened at the moment.

The error notification is deformed a bit now...
https://fedorapeople.org/~spichugi/error_boundaries.jpeg

It has an unnecessary slider at the bottom and, I think, it is a bit too long... Maybe we can make it fixed size and multiplied (I think it was like this but the error was just plain text)?

The interface for Importing and Exporting LDIFs seems odd to me... In the, CLI we can always check what is inside of /var/lib/dirsrv/slapd-instance_name/ldif/ directory. But in the UI, there is no short way to check it.
Can we add a simple file list for the LDIF directory?

Also, I think, there was a way optionally to set a sub-suffix to exclude or include during Import/Export.

The interface for Importing and Exporting LDIFs seems odd to me... In the, CLI we can always check what is inside of /var/lib/dirsrv/slapd-instance_name/ldif/ directory. But in the UI, there is no short way to check it.
Can we add a simple file list for the LDIF directory?

So speaking of cli consistency ... because dsctl got the commands "db2ldif" and "ldif2db" instead of concepts like "ldif_backup [create|restore]" we lost a valuable opportunity to create a "list" command here which could have filled this need :(

IMO if we are going to add "listing" like this, we should do it as the above example, and have it read the location from ds_paths nicely etc.

Also, I think, there was a way optionally to set a sub-suffix to exclude or include during Import/Export.

So speaking of cli consistency ... because dsctl got the commands "db2ldif" and "ldif2db" instead of concepts like "ldif_backup [create|restore]" we lost a valuable opportunity to create a "list" command here which could have filled this need :(

I think it makes sense. We'll see what Mark will say.

Another thing that we can think about @mreynolds... Can we load the data one time and then update as needed?
I think it is a bit annoying that it loads for a few seconds each time we switch between suffixes.

Probably we can load the data into some data-structure stored in stateand then pass it to the child components. I have something like this implemented in Plugins tab.
What do you think?

There are a couple of errors in the console log:
[DOM] Found 2 elements with non-unique id #find: (More info: https://goo.gl/9p2vKq)
​
​
[DOM] Found 2 elements with non-unique id #pagination-row-dropdown: (More info: https://goo.gl/9p2vKq)
​…​​
​…​​

But I am not sure why the IDs are not unique... The elements are patternfly-react objects.

Yeah I have no control over this - its all in PF-react

Another thing that we can think about @mreynolds... Can we load the data one time and then update as needed?
I think it is a bit annoying that it loads for a few seconds each time we switch between suffixes.
Probably we can load the data into some data-structure stored in stateand then pass it to the child components. I have something like this implemented in Plugins tab.
What do you think?

Having multiple suffixes it not very common, and refreshing the data when you switch between suffixes isn't that terrible. There are pros and cons to both approaches. One we load a fresh set of data, the other is faster but potentially outdated. Let me think about it, but probably loading it once is the lesser of two evils.

I think there is an "ensure" type on ldapobjects that does this style wrapping? It's probably better not to confuse "create" which is an action, with "ensure" which is stateful.

Sorry I'm not sure what you are concerned about here. I don't see what I am doing wrong. We always create a properties dict to create new objects. I must be missing your point, or I am looking at the wrong code.

So speaking of cli consistency ... because dsctl got the commands "db2ldif" and "ldif2db" instead of concepts like "ldif_backup [create|restore]" we lost a valuable opportunity to create a "list" command here which could have filled this need :(

I think it makes sense. We'll see what Mark will say.

Sounds good to me.

The error notification is deformed a bit now...
https://fedorapeople.org/~spichugi/error_boundaries.jpeg
It has an unnecessary slider at the bottom and, I think, it is a bit too long... Maybe we can make it fixed size and multiplied (I think it was like this but the error was just plain text)?

This can be improved. Fixed size sounds good. I did change it to a

 tag so the errors from the CLI are properly displayed, but we can always improve it.

      

Looks like the function only does elements[i].setAttribute('title', elements[i].innerText);... The rest is commented out.

This is a prototype for doing right click on treeview nodes - it is commented out because I did not want to lose the work I had already done. But I will remove it and save that commented code on my own system.

Can we remove the function and use the React state approach for updating the elements (if we need it at all)?

Not possible, treeview does not have any option to set the title, so it has to be added after it is rendered. Yes it's a hack, but I don't know how it could be done differently since we are using PF's treeview. If you know of a better way please let me know!

FYI, the purpose of this is so you can view long "suffixes" via titles in the tree view without consuming a large amount of horizontal space. Otherwise the suffix value gets truncated and it not readable.

@firstyear Oh okay I see what you are saying. However, In this case though I DO want to create the mapping tree - not just check if one exists. For chaining we have to create the mapping tree separately.

Ensure will "create" if it does not exist, and will "bring it into line" if it does exist. It avoids you needidng to do any checks for existance. Just say "ensure(properties)" and you know it MUST be there in the form you want.

Ensure will "create" if it does not exist, and will "bring it into line" if it does exist. It avoids you needidng to do any checks for existance. Just say "ensure(properties)" and you know it MUST be there in the form you want.

Yeah it was ensure_state(), and I've added it in. I expect to have the next rebase ready tomorrow, and it should address all comments.

All changes applied, and I also added a UI page to manage all backups and LDIFs.

Please review...

At the VLV Indexes tab, after I click on Create VLV Index, it opens Edit modal window and I can't insert anything to VLV Index Name

After loading the UI, I see a warning in the console log:

Warning: Can't perform a React state update on an unmounted component.
This is a no-op, but it indicates a memory leak in your application.
To fix, cancel all subscriptions and asynchronous tasks
in the componentWillUnmount method.
in SuffixIndexes (created by Suffix)
in TabContainer (created by Uncontrolled(TabContainer))
in Uncontrolled(TabContainer) (created by Suffix)
in div (created by Suffix)
in div (created by TabPane)
in Transition (created by Fade)
in Fade (created by TabPane)
in TabPane (created by Suffix)
in div (created by TabContent)
in TabContent (created by Suffix)
in div (created by Suffix)
in TabContainer (created by Uncontrolled(TabContainer))
in Uncontrolled(TabContainer) (created by Suffix)
in div (created by Suffix)
in Suffix (created by Database)

First time I see something like this...

Chaining Configuration - If I change Max TCP Connections and press Save Default Settings button nothing happens

Also, in the Suffix Settings - Database Read-Only Mode checkbox doesn't react on the click.

After loading the UI, I see a warning in the console log:
Warning: Can't perform a React state update on an unmounted component.
This is a no-op, but it indicates a memory leak in your application.
To fix, cancel all subscriptions and asynchronous tasks
in the componentWillUnmount method.

in SuffixIndexes (created by Suffix)
in TabContainer (created by Uncontrolled(TabContainer))
in Uncontrolled(TabContainer) (created by Suffix)
in div (created by Suffix)
in div (created by TabPane)
in Transition (created by Fade)
in Fade (created by TabPane)
in TabPane (created by Suffix)
in div (created by TabContent)
in TabContent (created by Suffix)
in div (created by Suffix)
in TabContainer (created by Uncontrolled(TabContainer))
in Uncontrolled(TabContainer) (created by Suffix)
in div (created by Suffix)
in Suffix (created by Database)

First time I see something like this...

After loading the UI, I see a warning in the console log:
Warning: Can't perform a React state update on an unmounted component.
This is a no-op, but it indicates a memory leak in your application.
To fix, cancel all subscriptions and asynchronous tasks
in the componentWillUnmount method.

in SuffixIndexes (created by Suffix)
in TabContainer (created by Uncontrolled(TabContainer))
in Uncontrolled(TabContainer) (created by Suffix)
in div (created by Suffix)
in div (created by TabPane)
in Transition (created by Fade)
in Fade (created by TabPane)
in TabPane (created by Suffix)
in div (created by TabContent)
in TabContent (created by Suffix)
in div (created by Suffix)
in TabContainer (created by Uncontrolled(TabContainer))
in Uncontrolled(TabContainer) (created by Suffix)
in div (created by Suffix)
in Suffix (created by Database)

First time I see something like this...

I see this if I switch between suffixes before they are done loading. I do not see it when I allow it to fully load or there is only one suffix

Fixed all the issues. As for setState after component is unmounted. I fixed the warning that we both saw in SuffixIndexes, but I did not incorporate this "mounted" check to all setState calls in all classes. That's just too much work - I think we should fix them as we find them. So far I have not seen this warning manifest itself in other ways.

Please review again...

I think 80 is too small and is not reasonable for current screens at all... Or was it removed unintentionally during the rebase?

You still have a couple of 'var' definitions left across the PR... Was it intentionally?

Besides that, looks good to me!

I think 80 is too small and is not reasonable for current screens at all... Or was it removed unintentionally during the rebase?

I removed the limit it because I was exceeding the line length and the warnings were unbearable. I did not set it to 80 - it just removed it.

Yes the few places I used "var" was intentional

I think 80 is too small and is not reasonable for current screens at all... Or was it removed unintentionally during the rebase?

I removed the limit it because I was exceeding the line length and the warnings were unbearable. I did not set it to 80 - it just removed it.

Okay, my formatter was cutting the lines into 80 by default. So I assumed it is the default. :)
I am okay with the reasonable expending.

Yes the few places I used "var" was intentional

For what I see, you use var only in a scope it was defined...
So let is a better choice there (because var is really weak and can mess things up - https://hackernoon.com/js-var-let-or-const-67e51dbb716f )

Do I miss something?

@spichugi Oh there were more vars than I realized. They should all be correct now, please check one last time...

LGTM! Ack

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3275

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.