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.

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

Disable resetting to the standard model #50048

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Conversation

bl-nero
Copy link
Contributor

@bl-nero bl-nero commented Dec 11, 2024

Until we come up with a more trustworthy user experience, we don't want to risk throwing the users under the bus by performing actions that are not detailed in the UI and that would be difficult to understand for the users because of the level of complication of the editor's model. Just display the message and disable the standard editor UI.

Screenshot 2024-12-11 at 10 58 17

@bl-nero bl-nero added no-changelog Indicates that a PR does not require a changelog entry backport/branch/v17 labels Dec 11, 2024
Copy link
Contributor

@flyinghermit flyinghermit left a comment

Choose a reason for hiding this comment

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

I have a suggestion for the wording in the comment, looks good otherwise.

Comment on lines 25 to 26
Some fields were not readable by the standard editor. To continue editing,
go back to YAML editor or reset the affected fields to standard settings.
go back to YAML editor.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Some fields were not readable by the standard editor" hints like some bug that I think does not provide any value to the user.

Maybe some variations of the two sentences below?

  1. Standard editor is not supported this time. To continue editing, please go back to the YAML editor.
  2. Standard editor is coming soon. To continue editing, please go back to the YAML editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"This time" suggest that it's a transient issue, and the user should try again later. "Coming soon" suggests that standard editor is not available at all. I think "This role is too complex to be edited in standard editor" will be the best message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we disabling standard editor by default or just in a few scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is just in these scenarios. #50067 bridges the gap close enough to support built-in editor and auditor roles, but the access role uses a bit that we're still unsure how to represent. Also, for simplicity, deny rules are dropped from this release.

@bl-nero bl-nero changed the base branch from bl-nero/role-editor-base to master December 12, 2024 14:36
@bl-nero bl-nero changed the base branch from master to bl-nero/role-editor-base December 12, 2024 14:37
@bl-nero bl-nero force-pushed the bl-nero/role-editor-no-reset branch from 693af12 to 431c84c Compare December 12, 2024 14:44
@bl-nero bl-nero changed the base branch from bl-nero/role-editor-base to master December 12, 2024 14:44
@bl-nero bl-nero force-pushed the bl-nero/role-editor-no-reset branch from 431c84c to bb12ddf Compare December 12, 2024 14:46
@bl-nero bl-nero enabled auto-merge December 12, 2024 15:37
@bl-nero bl-nero added this pull request to the merge queue Dec 12, 2024
auto-merge was automatically disabled December 12, 2024 15:47

Pull Request is not mergeable

Merged via the queue into master with commit b43a2d2 Dec 12, 2024
40 checks passed
@bl-nero bl-nero deleted the bl-nero/role-editor-no-reset branch December 12, 2024 16:03
@public-teleport-github-review-bot

@bl-nero See the table below for backport results.

Branch Result
branch/v17 Failed

bl-nero added a commit that referenced this pull request Dec 12, 2024
* Disable resetting to the standard model

* Review

* Test fix

* Fix a test
github-merge-queue bot pushed a commit that referenced this pull request Dec 13, 2024
* Disable resetting to the standard model

* Review

* Test fix

* Fix a test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v17 no-changelog Indicates that a PR does not require a changelog entry size/sm ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants