gchq / gaffer-tools

gaffer-tools is deprecated. Use https://github.com/gchq/gafferpy instead
Apache License 2.0
50 stars 29 forks source link

Add labels/tags to named operations #791

Closed p013570 closed 4 years ago

p013570 commented 5 years ago

Often users want to be able to create a group of named operations and for a particular use case. At the moment this tends to be done by adding a prefix to the operation name. It would be better to allow labels to be added to named operations. Then the UI could display the named operations in different sections in the dropdown and it would be easier to search for the named operations you want. Also the UI could then white/black list a particular group of named operations. This is also useful when used with the FederatedStore and there are multiple graphs involved and users want to write different named operations for the different graphs.

macenturalxl1 commented 4 years ago

I can claim this one.

Do you want the word 'label' to be used throughout, or can I rename it as 'groupName' or 'groupNameLabel' or 'groupLabel' (or other you prefer)? Just when I first read it, the word label can be confusing or too generic to what it refers to and expected behaviour.

Unless label has some other functionality or reason I'm not aware of.

macenturalxl1 commented 4 years ago

Then the UI could display the named operations in different sections in the dropdown and it would be easier to search for the named operations you want.

Is this screen shot what you had in mind for this part of the Issue?

Grouped Operations Screenshot

When you add a label to a named operation, it will be dynamically prefixed to the name as "[group label name] Operation Name" - if no label is added then it will display the name only in the list - so it is kind of staying in line when it is manually prefixed in the name as before and simple.

Or would you expect something visually different how grouped Named Operations should be displayed in the dropdown? (if this design is ok for now then it can always be enhanced shortly after as the feature matures)

t11947 commented 4 years ago

I think this is a good solution if the operations were limited to one label only, but could become a little cluttered when dealing with multiple labels per operation. Perhaps listing the labels on a third line, after the description. It might be worth also adding search by label and an option to group the list by label, too, but perhaps this is creeping out of scope.

macenturalxl1 commented 4 years ago

I have had feedback to handle multiple labels on the Pull Request and can see now what was meant from the Issue requirement above - so I'll implement your recommended design of adding a list of labels on a third line. I'll make them searchable too using the current dropdown box - as the Operation's description is searchable too in this way.

As for the feature to group the list by label - I'm not sure just yet on how best to design it (as there are many ways to implement this). I'll have a think and let me know of any recommendations. Like you say, this could be scoped out to a second Issue if more design thinking is needed

jpelbertrios commented 4 years ago

I can calim it for design

macenturalxl1 commented 4 years ago

For the whiteList/blackList by label feature...

Would you simply just add the label to one of the arrays along side Operation names?

E.g. if I want to whiteList all ops with the label My public label and blackList Banned label, then the config.json will look something like this?

  "operations": {
    "whiteList": ["My public label"],
    "blackList": ["BannedNamedOperation", "Banned label"],
    "loadNamedOperationsOnStartup": true
  },

And when making the request to GetAllNamedOperations, the UI (operation-service) will filter all out with the label Banned label?

macenturalxl1 commented 4 years ago

Hi - I've also been thinking about the combination of listed names and labels in the config and rules to manage conflicts.

I think the general rule could be:

Here are the possible outcomes for the following Operations according to those rules, feel free to edit the outcomes (or add any missed) for the following Operation combinations and I’ll implement tests/implementation:

Key: (W) = whitelisted, (B) = blacklisted, (n) = name or label not listed in config

Name (n)
Labels (n)(n)]
= Allowed
Name (W)
Labels (n)(n)
= Allowed
Name (n)
Labels (W)(W)
= Allowed
Name (W)
Labels (W)(W)
= Allowed
Name (n)
Labels (B)(B)
= Not allowed
Name (B)
Labels (n)(n)
= Not allowed
Name (B)
Labels (B)(B)
= Not allowed

*White/blacklist combinations:

Name (W)
Labels (B)(B)
= Allowed
Name (W)
Labels (B)(W)
= Allowed
Name (B)
Labels (W)(W)
= Allowed 
Name (B)
Labels (B)(W)
= Allowed
Name (n)
Labels (B)(W)
= Allowed
d47853 commented 4 years ago

In my opinion, that's mostly right except that I think a whitelisted name should overwrite everything, even blacklisted labels. I feel that if the person specifically added an operation name to a whitelist, they would be certain that they wanted it included in the UI.

macenturalxl1 commented 4 years ago

In my opinion, that's mostly right except that I think a whitelisted name should overwrite everything, even blacklisted labels. I feel that if the person specifically added an operation name to a whitelist, they would be certain that they wanted it included in the UI.

That makes sense and is a nice simple rule for the User to understand easily - that a whitelisted name/label overrides and will be make the Operation visible (see updated outcomes comment above) - I'll implement this and update when completed

d47853 commented 4 years ago

Merged into develop