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

Consistently handle the 'disabled' attr in inputs #2929

Open
wants to merge 1 commit into
base: develop
from

Conversation

@abeluck
Copy link

abeluck commented Feb 11, 2020

As discussed in https://community.zammad.org/t/automating-creation-of-custom-object-attributes/3831/5

This commit ensures that the generic input widgets handle the disabled
attribute consistently. Before only the checkbox, select and input widgets respected
this attribute.

I've added it to the following widgets

  • tree/searchable select
  • textarea
  • datetime
  • date

Now it is possible to define a screens policy like so:

screens: {
"edit"=>{
"ticket.customer"=>{"shown"=>true, "required"=>false},
"ticket.agent"=>{
  "shown"=>true,
  "required"=>false,
  "disabled"=>true # <--- add this
}}}

Then the fields will show up on the Agent's sidebar with the customer
submitted values, but the Agent will not be able to change the values.

This is not the same as real permissions, as presumably a manually
crafted API request could circumvent the disabled attribute.

But this is a first step towards that direction, and very useful for us
in prod right now.

@thorsteneckel

This comment has been minimized.

Copy link
Collaborator

thorsteneckel commented Feb 11, 2020

Hey @abeluck - wow! This was like instantly fast. Would you mind sharing some screenshots on how the elements look when they are disabled?

@abeluck

This comment has been minimized.

Copy link
Author

abeluck commented Feb 11, 2020

Your response was instantly fast :) I was in the process of creating a screenshot while you typed that :)

zammad-consistent-disabled

"Custom attribute" and "Test_ttset" are both intentionally not disabled.

@thorsteneckel

This comment has been minimized.

Copy link
Collaborator

thorsteneckel commented Feb 11, 2020

That looks great! Would you also mind to add some tests to cover this behavior? public/assets/tests/form.js would be the file. We'd be happy to answer your questions.

@abeluck

This comment has been minimized.

Copy link
Author

abeluck commented Feb 11, 2020

Sure!

How could I run just those frontend tests? rake test seems to run the whole suite.

@thorsteneckel

This comment has been minimized.

Copy link
Collaborator

thorsteneckel commented Feb 11, 2020

The most convenient way is to start Zammad in development mode and navigate your browser to http://localhost:3000/tests_form 🚀

@abeluck

This comment has been minimized.

Copy link
Author

abeluck commented Feb 11, 2020

Gotcha, thanks.

I'm having trouble finding a test that will capture the whole thing from adding disabled: true in the screens to rendering it disabled... so that I can modify it for this usecase :)

Is there an existing test that takes a object attribute params object and renders it? The closest I see are object manager form 1/2/3 but those don't render the final form the agent sees.. just the admin ui.

If you could point me to it, then I can base my test on it.

@abeluck abeluck force-pushed the abeluck:develop branch from 66fb4a3 to af4fe38 Feb 11, 2020
@abeluck

This comment has been minimized.

Copy link
Author

abeluck commented Feb 11, 2020

I added basic render tests (see new changes) that use the "disabled" attribute in the form params object. This works great!

Along the way I added disabled support for radiobox, richtext, and checkbox just to be consistent, though ObjectManager::Attributes cannot have these types. I intentionally did not add disabled support for password or active as I don't think it makes sense for those.

If you also want a test that tests all the way from the ObjectManager::Attribute screens hash, then I'll need some help as per my previous comment.

As discussed in https://community.zammad.org/t/automating-creation-of-custom-object-attributes/3831/5

This commit ensures that the generic input widgets handle the `disabled`
attribute consistently. Before only the checkbox, select and input widgets respected
this attribute.

I've added it to the following widgets

* tree/searchable select
* textarea
* datetime
* date
* richtext: added appropriate css to hide the attachment link and
            grey-out the div
* checkbox
* radio

Now it is possible to define a screens policy like so:
```
screens: {
"edit"=>{
"ticket.customer"=>{"shown"=>true, "required"=>false},
"ticket.agent"=>{
  "shown"=>true,
  "required"=>false,
  "disabled"=>true # <--- add this
}}}
```

Then the fields will show up on the Agent's sidebar with the customer
submitted values, but the Agent will not be able to change the values.

This is not the same as real permissions, as presumably a manually
crafted API request could circumvent the disabled attribute.

But this is a first step towards that direction, and very useful for us
in prod right now.
@abeluck abeluck force-pushed the abeluck:develop branch from af4fe38 to 7285541 Feb 11, 2020
@abeluck

This comment has been minimized.

Copy link
Author

abeluck commented Feb 11, 2020

Here are screenshots of the new widgets

zammad-disabled-check
zammad-disabled-radio

I didn't like how the richtext edit still looked editable, even though it wasn't, so I added some appropriate css and hid the "select attachment" link.

before css (it looks editable even though it isn't):
zammad-disabled-contenteditable

after css (now it looks properly disabled):
zammad-disabled-contenteditable2

@thorsteneckel

This comment has been minimized.

Copy link
Collaborator

thorsteneckel commented Feb 11, 2020

Damn sweet! @mantas could you please have a look and create a MR to check the impact on CI?

@abeluck

This comment has been minimized.

Copy link
Author

abeluck commented Feb 20, 2020

@mantas @thorsteneckel I know a release is imminent, and chance this PR and/or Fix deserialization of multiselect value #2932 could make it in?

@thorsteneckel

This comment has been minimized.

Copy link
Collaborator

thorsteneckel commented Feb 20, 2020

@mantas could you please prioritize this PR?

@mantas

This comment has been minimized.

Copy link
Collaborator

mantas commented Feb 20, 2020

@thorsteneckel sure, I'll look into this today

@mantas

This comment has been minimized.

Copy link
Collaborator

mantas commented Feb 20, 2020

Hi @abeluck ,

Unfortunately this PR won't make it to the upcoming release. While PR itself is great, it brought our attention to a semi-related part of the codebase that may need some love before pulling this one in. We hope to include this in next release.

Sorry for the delay.

@abeluck

This comment has been minimized.

Copy link
Author

abeluck commented Feb 20, 2020

Understood. Looking forward to the next release then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.