Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

UI policy imports #1793

Merged
merged 6 commits into from
Nov 21, 2023
Merged

UI policy imports #1793

merged 6 commits into from
Nov 21, 2023

Conversation

thfries
Copy link
Contributor

@thfries thfries commented Nov 5, 2023 •
edited
Loading

please find this bigger pull requests for policy imports (fixes #1700). Would be nice to get feedback from users that use policy imports.

Also a "full json" editor for the policy was added. Including some templates for policies (I took some from the documentation but may be other templates make sense. e.g. is there a "minimal policy"?)

The overall status is stable. Two things could be improved in future

  • Creating a policy is only possible on the json editor
  • Allow to create a new import from the "recent policies" table and allow to create a new subject from the "who am I" table

@thjaeckle thjaeckle added the UI Issues related to the Ditto explorer UI label Nov 5, 2023
thjaeckle
thjaeckle reviewed Nov 6, 2023
View reviewed changes
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @thfries for the policy imports addition to the UI.
I must say that I really like it - the thoughts you put into selecting the checkboxes for importing "implicit", "explicit" and "never" marked policy entries shows that you really understood the approach 👍 :)
I also really like the "Recent policies" idea.

I have a few remarks which might be worth to consider:

  • the tabs for policies are now: "CRUD Policy" and "Policy JSON"
    • for things however, the tabs with similar names have different meaning: "CRUD" for things shows the JSON Editor
    • so for "CRUD Policy" I was also expecting the JSON editor ..
    • maybe this can be solved by also renaming the Things "CRUD" tab to e.g. just "JSON"
    • the Policy tabs I would then e.g. name: "Manage" and "JSON"
    • or another idea:
      • have 3 Tabs: "Imports", "Entries" and "JSON"
      • Both, Policy imports and entries can become quite long and I don't see that it really is required to have them on the same tab
  • the heading "PolicyImports" misses a whitespace
  • the label showing "ImportPolicyId" I would rather change to "Imported Policy ID"
  • for creating new "Policy Imports", "Entries", "Subjects" or "Resources" in the "CRUD Policy" tab, the Button should have the label "Create" instead of "Edit" when a new entry can be added
    • the same would also be good for Thing features to have it behave equally

Nice to have:

  • what I missed - even more than the 2 suggestions you provided - is the ability to navigate to a imported Policy
    • I would suggest to do that in similar way than the clipboard icon in the "Recent policies" list: Provide an icon at the right of each "Policy import" with an arrow or so - when clicking on it, load the imported policy as currently "selected" policy
    • the previously selected policy would automatically still be available in the "Recent policy" list

@thjaeckle thjaeckle added this to the 3.5.0 milestone Nov 6, 2023
@thfries
Copy link
Contributor Author

thfries commented Nov 7, 2023

Thanks for the feedback. Agree to all points. I like the idea with the three tabs.
I m going to work on that...

@thfries
Copy link
Contributor Author

thfries commented Nov 11, 2023

Hi @thjaeckle,
all done. Happy if you could take a look again.
I expect some conflicts after the merge for the piggyback commands, so we may also do a final check after this merge was done.

@thjaeckle
Copy link
Member

Thanks a ton, @thfries
I will review as soon as possible. Looking forward to it :)

@thfries
UI - Support for policy imports
3c4912e 
Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
UI - policy imports review
733dd2f 
- Changed naming CRUD, JSON and Manage
- Fixed typos in labels
- Splitted tabs for entries and imports
- Fixed things search field delete button
- Search button now in primary color
- Empty crud toolbar now called create instead of edit

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
UI - policy imports review 2
876c30b 
- Update bootstrap-icons for new copy icon
- Fixed color of selected copy icon
- Allow additional actions in table rows
- Clipboard copy value now explicit and not guessed from sibling html
- Policy import allows navigation
- fixed SSE mapping of incoming messages

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
UI - Send messages improvements
47ea395 
- disable send buttons when no thing or feature is selected

Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
UI - policy imports missing headers in new files
b9d874d 
Signed-off-by: thfries <thomas.fries0@gmail.com>
@thjaeckle
Copy link
Member

thjaeckle commented Nov 19, 2023 •
edited
Loading

Hi @thfries
The reworked Policy-Page looks great - I have nothing to add, great work. 👍

One thing:

  • I thought that you will also adjust the "Manage" tab on the Things page (and also for Feature) to also name in "JSON" (same as now in the Policy page) .. for consistency.

In addition, I noticed a weird behavior in the "Things" table. At least when I start Ditto locally and connect the UI to that, I cannot select any longer any thing - for some things I will get an error in the SSE (canceled) which is tried to be opened when selecting another Thing.
Some Things work however - I do not really understand, why.
Did you change anything in the Things table? Otherwise this does not hinder approving this PR.

*Edit: On "master" this is not a problem (connecting to the same local Ditto), so apparently this was introduced in this PR.

@thfries
Copy link
Contributor Author

thfries commented Nov 21, 2023

oh, no. This must be a caused by a change in 876c30b on messagesIncoming.ts. I was forced by typescript to change there. I missed that, sorry. Let me check.

I renamed

  • "CRUD" to "Manage" for things and features
  • "CRUD" to "Imports" and "Entries" for policies

We could use "JSON" instead of "Manage" but is it then clear that you can create, edit and delete entities there? But I also see this inconsistency...

@thfries
UI - bug fix for things without features
117389c 
Signed-off-by: thfries <thomas.fries0@gmail.com>
@thfries
Copy link
Contributor Author

thfries commented Nov 21, 2023

Hi @thjaeckle,

I fixed the issue. Reason for this bug was something different.

Very annoying that these types of errors are just dropped silently. More sustainable fix would be to introduce an interface for Thing with optional feature property and make typescript discover that bug.

Was about to change "Manage" to "JSON", too. But on the Environments page, there is "Manage" and "JSON" already next to each other... So no idea here

thjaeckle
thjaeckle approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@thjaeckle thjaeckle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thfries cool, the bug is fixed 👍

Right, I also agree that "Manage" does make more sense in some cases.
So I think the way it currently is, is ok 👌

Would merge this PR if nothing else comes up.

Thanks a lot for the contribution.

@thjaeckle thjaeckle merged commit 619913c into eclipse-ditto:master Nov 21, 2023
3 checks passed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thjaeckle thjaeckle thjaeckle approved these changes