Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Right now, you can add new configurations but not edit or remove them.
Proposed resolution
Change the labels to selects so they can be changed. Either add a delete button or remove rows without a selection.
Also, once done, provide a screenshot that can be used for the project page.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#23 | allow_edit_and_remove-2681131-23.patch | 40.57 KB | tduong |
| |||
#23 | allow_edit_and_remove-2681131-23-test_only.patch | 7.8 KB | tduong |
#23 | interdiff-2681131-20-23.txt | 13.82 KB | tduong |
#20 | after.png | 196.54 KB | tduong |
#20 | before.png | 202.9 KB | tduong |
Comments
Comment #2
tduong CreditAttribution: tduong at MD Systems GmbH commentedChanged that tableselect to table, added Remove column with checkboxes to delete the custom configuration rows.
Comment #3
BerdirLets me sure our tests cover those operations.
Comment #5
tduong CreditAttribution: tduong at MD Systems GmbH commentedTried to fix the test, but for the PHPUnit test I'm not sure what does that
'module_one' => 'not_clean'
mean...Anyway, I've extended a test to check updating and removing custom modules.
However while manually testing to 'create' new custom modules with the same settings as one of the existing custom modules (no key specified), instead of updating it, it simply doesn't do anything, while with the old code it updates it correctly... we should check what happens exactly and fix it!
Comment #7
tduong CreditAttribution: tduong at MD Systems GmbH commentedDebugged it and found that if you already have some custom module configurations in the table and you want to update the e.g. one of these configurations "from outside the table", it set your values first correctly, but then it always goes into the last check (
if ($form_state->hasValue(['custom', 'modules']) && is_array($form_state->getValue(['custom', 'modules'])))
, since the configurations already exist, thus that table have values and it's always TRUE), then it takes the values that your configuration had before and set it back again. As result it seems like nothing happened.But otherwise updating form the table worked fine!
So the problem is that updating from
custom[custom_formatter], custom[custom_sender]
doesn't actually updatecustom[modules][<em>module.key</em>][formatter], custom[modules][<em>module.key</em>][sender]
. Does anyone know how to actually update it or can suggest any other way to avoid double "updating" these configurations? Otherwise I think that it might be better to have either another button/action link to separate updating configurations operation from the main "Save configuration" button or an "Operation" column (instead of "Remove") and a dropdown operation buttons as "Update" and "Delete".Is this approach ok ?
Comment #8
tduong CreditAttribution: tduong at MD Systems GmbH commentedMaybe this would be better to be in a follow-up instead, and let this issue be only related to how the table works, which is currently ok I guess.
Comment #9
tduong CreditAttribution: tduong at MD Systems GmbH commentedBack to this issue. I've added a (kind of) flag to be able to check if an update of the custom configuration is needed or not + refactoring.
Still not sure about the
'module_one' => 'not_clean'
thing, mentioned in #5.Comment #11
BerdirStarted looking into this, trying to simplify.
Comment #12
BerdirTwo things made this complicated.
a) That we were trying to support two different ways to update entries. Both through the form above and with the new form elements in the table.
b) The mess in the existing code.
That frustrated me too much, so I rewrote it all ;)
Now the upper form can only be used to add new, not existing entries, it has useful validation and a separate add button. And I cleaned up the existing code *a lot*.
Didn't update the tests yet, \Drupal\Tests\mailsystem\Unit\AdminFormTest will fail badly, but I'm considering to remove most of that or even everything. We have a UI test now, that's much more useful to make sure that the form actually works, the unit test will just do what we tell it to on the code-level. Lets make sure we have good web tests for the interaction and then lets discuss what to do with AdminFormTest.
Comment #14
tduong CreditAttribution: tduong at MD Systems GmbH commentedFixed and updated both tests for now. Tomorrow we will discuss about the things mentioned above.
Comment #17
tduong CreditAttribution: tduong at MD Systems GmbH commentedUploaded patch:
validateAdd()
Comment #20
tduong CreditAttribution: tduong at MD Systems GmbH commentedReverted check in
validateAdd()
trying to update an existing custom module and uploaded some screenshots.Comment #23
tduong CreditAttribution: tduong at MD Systems GmbH commentedDiscussed with @berdir. Added test/check for theme options and module option labels in the UI test from the unit test, dropped unit test.
Comment #26
BerdirGreat, committed :)