The current state of the PR is intermediate. I'll add 'fixup' task and 'config entry - manage' part i a couple of following days.
But please, review the rest. It is pretty big already...

Also, you need to reinstall the node_modules for 'react-bootstrap-typeahead' package.

This shouldn't be choices, because it could be any oc that takes MO.

What's an example of this command in use? dsconf localhost plugin memberof edit --attr ?

Okay, I'm not sure about generic edit. I'm currently working on "modify" in another ticket, that will perform the same. Part of the design, of having the ability to "wrap and set" attributes (which you have removed in this patch), is that it's really clear what can and can't be altered in a really natural way. Another aspect is the cli should do "lots of common tasks" but "not everything". We should be offering only common useful settings, rather than everything. The cli offers a safe modification interface.

A benefit of your change is "batch" changes, where you change many attributes at once. However, I would prefer that there is a "delete" keyword rather than "no string" meaning delete. I think that deletes should always be a requested action, not an implied actionn. The design of this --edit will mean any mistake on the persons part where they forgot to write an argument will cause harm. Any situation where a person could "hold it wrong" means we have failed as tool designers.

So I would say these are the options I request: leave the CLI as is and use the "modify" interface that I'm adding, which does what you want for "advanced" users, and has a better, clearer delete syntax.

Or, implement this change, but have a clear "delete" keyword when you want to remove an attribute, rather than the current method of empty string."--attr " // "--attr delete". Enforce that each --attr takes 1 or more arguments (if more is valid).

This shouldn't be choices, because it could be any oc that takes MO.

Okay, it had these choices before - so I left it as is. But I think you are right.
A user can add some custom schema. I'll change it to the text field in the UI.

What's an example of this command in use? dsconf localhost plugin memberof edit --attr ?

Yes

Okay, I'm not sure about generic edit. I'm currently working on "modify" in another ticket, that will perform the same. Part of the design, of having the ability to "wrap and set" attributes (which you have removed in this patch), is that it's really clear what can and can't be altered in a really natural way. Another aspect is the cli should do "lots of common tasks" but "not everything". We should be offering only common useful settings, rather than everything. The cli offers a safe modification interface.

I think the way I have it (and other CLI tools that Mark was implementing too - we use the same way everywhere) is also able to limit a user. Like, you can edit only the attributes that were explicitly specified in the '--' flags. If the administrator would like to set up something, he will open the help and he will see what can be set.
I agree with you. We shouldn't include everything in the explicit option list. There are few attributes that we can have in plugin entry that I didn't add directly to CLI (pluginarg0, etc.) So I always try to filter to the way you said.

A benefit of your change is "batch" changes, where you change many attributes at once. However, I would prefer that there is a "delete" keyword rather than "no string" meaning delete. I think that deletes should always be a requested action, not an implied actionn. The design of this --edit will mean any mistake on the persons part where they forgot to write an argument will cause harm. Any situation where a person could "hold it wrong" means we have failed as tool designers.
So I would say these are the options I request: leave the CLI as is and use the "modify" interface that I'm adding, which does what you want for "advanced" users, and has a better, clearer delete syntax.
Or, implement this change, but have a clear "delete" keyword when you want to remove an attribute, rather than the current method of empty string."--attr " // "--attr delete". Enforce that each --attr takes 1 or more arguments (if more is valid).

Okay, I was thinking about that too. I will change it.

So in general, the way with batch changes is the way we have it now for some time. Mark and I was doing it for other parts of the CLI and I think we should keep at least because of the two reasons:
1) Consistensy
2) CLI -> UI interactions. Currently, we call the CLI command in the Cockpit. And we better have it as a batch change because a calling the OS command (cockpit.spawn) is a pretty expensive operation and we try to do it as less times as possible.

Using "print" was on purpose. I think you can not redirect the output using log.info(). There was some problem with it that was only fixed by using print for CLI output.

This shouldn't be choices, because it could be any oc that takes MO.

Okay, it had these choices before - so I left it as is. But I think you are right.
A user can add some custom schema. I'll change it to the text field in the UI.

Just don't try and enforce anything, just allow any objectClass here, but recommend that nsMemberOf is probably what they want in most cases.

Or, implement this change, but have a clear "delete" keyword when you want to remove an attribute, rather than the current method of empty string."--attr " // "--attr delete". Enforce that each --attr takes 1 or more arguments (if more is valid).

Okay, I was thinking about that too. I will change it.
So in general, the way with batch changes is the way we have it now for some time. Mark and I was doing it for other parts of the CLI and I think we should keep at least because of the two reasons:
1) Consistensy
2) CLI -> UI interactions. Currently, we call the CLI command in the Cockpit. And we better have it as a batch change because a calling the OS command (cockpit.spawn) is a pretty expensive operation and we try to do it as less times as possible.

Yeah. Batching makes sense here. Check my patch in https://pagure.io/389-ds-base/pull-request/50158 because I think you can use this instead of generic edit here. Perhaps what is needed is a way to display from the "objectclass" the valid attributes from the schema (Which is why it's important we stop using extensible object ...).

Saying this, the two could be complimentary, rather than exclusive as well. We could keep edit with the list of valid options, and detailed help, and have modify as a raw interface to the objects? That could also get confusing in some cases too ....

PS: My patch in 50158 fixes some behaviour in mod_delete actions, so you probably will need to rebase and test this. :)

Yeah. Batching makes sense here. Check my patch in https://pagure.io/389-ds-base/pull-request/50158 because I think you can use this instead of generic edit here. Perhaps what is needed is a way to display from the "objectclass" the valid attributes from the schema (Which is why it's important we stop using extensible object ...).
Saying this, the two could be complimentary, rather than exclusive as well. We could keep edit with the list of valid options, and detailed help, and have modify as a raw interface to the objects? That could also get confusing in some cases too ....
PS: My patch in 50158 fixes some behaviour in mod_delete actions, so you probably will need to rebase and test this. :)

Okay, I'll check it next thing. Thanks!
Currently, I've just added new functionality for the 'fixup' and 'configArea' stuff.
So it's ready for the final review (after I'll look into the issues mentioned above)

Great, thanks! Let me know what you think of that PR as well. I can't say much about the JS here so I'll leave @mreynolds for that part :)

PS: We still should think about modify vs edit interface for these calls by the way. I think that perhaps on reflection, leave the current code (so roll back your py change), then have the js use the modify interface in 50158? Would that be more complex? Or better? It allows you to perform arbitrary modifications you want ....

PS: We still should think about modify vs edit interface for these calls by the way. I think that perhaps on reflection, leave the current code (so roll back your py change), then have the js use the modify interface in 50158? Would that be more complex? Or better? It allows you to perform arbitrary modifications you want ....

I think it is better with options (as we have consistently over most CLI areas that Mark and me have added). It gives nice discoverability with --help. And I think it is more natural for the user to specify flags instead of a new non-standard syntax. At least, for the most cases.
And I think it's too late to change the approach and rewrite the rest of the CLI (if we want to have it consistent)

Using "print" was on purpose. I think you can not redirect the output using log.info(). There was some problem with it that was only fixed by using print for CLI output.

Right... I didn't use the redirection with these tools but someone could need to.
Fixed (back to print)

Please, review!

PS: We still should think about modify vs edit interface for these calls by the way. I think that perhaps on reflection, leave the current code (so roll back your py change), then have the js use the modify interface in 50158? Would that be more complex? Or better? It allows you to perform arbitrary modifications you want ....

I think it is better with options (as we have consistently over most CLI areas that Mark and me have added). It gives nice discoverability with --help. And I think it is more natural for the user to specify flags instead of a new non-standard syntax. At least, for the most cases.
And I think it's too late to change the approach and rewrite the rest of the CLI (if we want to have it consistent)

This is a reasonable concern. I think it's hard because I have a different approach, but to make everything consistent would be breaking.

So how about we do the "edit" as you have here, and the "modify" from my other patch?

Otherwise, with this in mind, I'm happy with the python. Again, please rebase to my change in 50158 due to delete semantics, I'll let @mreynolds review the js. Thank you for being patient with me :)

Hi Simon, so there are some issues around "saving" the configuration because you are not maintaining a before and after snapshot of the settings. So if I do nothing and click save it actually rewrites the plugin entry in DS.

What I do here is I maintain two sets of the state attributes. Current and original, then on save I check if current is different than the original. If they are different I update the cmd list and perform the update. Something like this:

this.state({
    memberOfScope,
    _memberOfScope
})
if (this.state.memberOfScope != this.state._memberOfScope) {
    cmd.push{"--moscope=" + this.state.memberOfScope);
}

So when I "load" the config I set both state attributes, then "handleChange" updates the non "_" variable. Finally we compare them at "save" time, and only update what changed (if anything changed at all)

Also fixup task modal does not reset when it is reopened. If I put a value in the modal and cancel, then reopen it, the value is still present in the input field.

Also the "Config Entry" field, should be "Shared Config Entry".

The rest looks great!

Also, after making a config change (success or failure) I reload the config (which re-renders everything with the correct values and resets previous values ( --> the "_" state variables).

Also, by reloading the config after a successful or failed update means you don't have to maintain the html after a update because its always reset(re-rendered) from scratch. Well that's how I'm doing it in the database tab, and it works nicely, and actually creates less work.

Nice catch!

Yeah, actually I've noticed the issue before I switched to the docs and presentations work.
But I really like your solution a lot! It is very much reactive :) I'll use it. Thank you!

Okay, I fixed it a bit differently.
I think it is better to check just before the update on the server (so I fixed it in the lib389-CLI code). Too many things can happen between UI loading and the change. So I think the 'state' approach is not so reliable...
And we need the functionality in the CLI too anyway.

The rest is fixed too.

Well I disagree with your stance on using state to know if a value changed. Typically configuration is very static, and is only changed by one person. The odds of the memberOf plugin being changed between you opening the plugin (which should get fresh values), and clicking save is very slim.

Now, you are performing many operations even if the config did not change. My other concern here is attributes without values (or missing attributes), you will try to save empty-values which will fail for some attribute syntax checks (DirectoryString verses IA5String). In this case it does it correctly, but could this approach could fail for other configurations?

That being said, both approaches have pro's and con's - just proceed with caution if you continue to use this approach for other areas of the server.

Ack!

Well I disagree with your stance on using state to know if a value changed. Typically configuration is very static, and is only changed by one person. The odds of the memberOf plugin being changed between you opening the plugin (which should get fresh values), and clicking save is very slim.

I agree. I just wanted to have more precautions... Anyway, in the future, I'd like to have the UI to be in sync with the instance.

Now, you are performing many operations even if the config did not change. My other concern here is attributes without values (or missing attributes), you will try to save empty-values which will fail for some attribute syntax checks (DirectoryString verses IA5String). In this case it does it correctly, but could this approach could fail for other configurations?

I also check that we don't save any empty values. If the user set the field to an empty value, I assume the user would like to remove/not have the attribute in the entry.

That being said, both approaches have pro's and con's - just proceed with caution if you continue to use this approach for other areas of the server.
Ack!

Thanks!

I also check that we don't save any empty values. If the user set the field to an empty value, I assume the user would like to remove/not have the attribute in the entry.

What happens when it tries to delete an attribute that is not present in the entry? Is the error ignored, or is the operation rejected?

I also check that we don't save any empty values. If the user set the field to an empty value, I assume the user would like to remove/not have the attribute in the entry.

What happens when it tries to delete an attribute that is not present in the entry? Is the error ignored, or is the operation rejected?

CLI will filter the attribute from the modlist:
- delete operation
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/cli_conf/init.py#_56
- replace operation
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/cli_conf/init.py#_61

What happens when it tries to delete an attribute that is not present in the entry? Is the error ignored, or is the operation rejected?

CLI will filter the attribute from the modlist:

Great!

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/3234

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.