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

CommentFileSizeAuthor
#23 allow_edit_and_remove-2681131-23.patch40.57 KBtduong
#23 allow_edit_and_remove-2681131-23-test_only.patch7.8 KBtduong
#23 interdiff-2681131-20-23.txt13.82 KBtduong
#20 after.png196.54 KBtduong
#20 before.png202.9 KBtduong
#20 allow_edit_and_remove-2681131-20.patch30.37 KBtduong
#20 allow_edit_and_remove-2681131-20-test_only.patch10.44 KBtduong
#20 interdiff-2681131-17-20.txt902 bytestduong
#17 allow_edit_and_remove-2681131-17.patch30.5 KBtduong
#17 allow_edit_and_remove-2681131-17-test_only.patch10.44 KBtduong
#17 interdiff-2681131-14-17.txt7.37 KBtduong
#14 allow_edit_and_remove-2681131-14.patch26.92 KBtduong
#14 allow_edit_and_remove-2681131-14-test_only.patch7.04 KBtduong
#14 interdiff-2681131-12-14.txt6.99 KBtduong
#12 allow_edit_and_remove-2681131-12.patch23.25 KBBerdir
#9 allow_edit_and_remove-2681131-9.patch13.7 KBtduong
#9 interdiff-2681131-7-9.txt4.96 KBtduong
#7 allow_edit_and_remove-2681131-7.patch12.02 KBtduong
#7 interdiff-2681131-5-7.txt4.58 KBtduong
#5 allow_edit_and_remove-2681131-5.patch9.06 KBtduong
#5 allow_edit_and_remove-2681131-5-test_only.patch3.5 KBtduong
#5 interdiff-2681131-2-5.txt5.23 KBtduong
#2 allow_edit_and_remove-2681131-2.patch5.35 KBtduong
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

tduong’s picture

Assigned: Unassigned » tduong
Status: Active » Needs review
FileSize
5.35 KB

Changed that tableselect to table, added Remove column with checkboxes to delete the custom configuration rows.

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Lets me sure our tests cover those operations.

The last submitted patch, 2: allow_edit_and_remove-2681131-2.patch, failed testing.

tduong’s picture

Tried 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!

The last submitted patch, 5: allow_edit_and_remove-2681131-5-test_only.patch, failed testing.

tduong’s picture

Debugged 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 update custom[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 ?

tduong’s picture

Maybe 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.

tduong’s picture

Back 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.

Status: Needs review » Needs work

The last submitted patch, 9: allow_edit_and_remove-2681131-9.patch, failed testing.

Berdir’s picture

Assigned: tduong » Berdir

Started looking into this, trying to simplify.

Berdir’s picture

Assigned: Berdir » Unassigned
Status: Needs work » Needs review
FileSize
23.25 KB

Two 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.

Status: Needs review » Needs work

The last submitted patch, 12: allow_edit_and_remove-2681131-12.patch, failed testing.

tduong’s picture

Fixed and updated both tests for now. Tomorrow we will discuss about the things mentioned above.

The last submitted patch, 14: allow_edit_and_remove-2681131-14-test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: allow_edit_and_remove-2681131-14.patch, failed testing.

tduong’s picture

Uploaded patch:

  • improved validateAdd()
  • improved and extended the UI test to cover more cases
    • add custom module without key specified
    • try to add a custom module without setting a) module, b) formatter and sender, c) module formatter and sender
    • try to update an existing custom module from the upper form

The last submitted patch, 17: allow_edit_and_remove-2681131-17-test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: allow_edit_and_remove-2681131-17.patch, failed testing.

tduong’s picture

Reverted check in validateAdd() trying to update an existing custom module and uploaded some screenshots.

The last submitted patch, 20: allow_edit_and_remove-2681131-20-test_only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 20: allow_edit_and_remove-2681131-20.patch, failed testing.

tduong’s picture

Discussed with @berdir. Added test/check for theme options and module option labels in the UI test from the unit test, dropped unit test.

The last submitted patch, 23: allow_edit_and_remove-2681131-23-test_only.patch, failed testing.

  • Berdir committed e341042 on 8.x-4.x authored by tduong
    Issue #2681131 by tduong, Berdir: Allow to edit and remove per module/...
Berdir’s picture

Status: Needs review » Fixed
Issue tags: -Needs tests

Great, committed :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.