@escopecz It's my first time doing some design stuff in Mautic, so I'd be grateful to get some suggestions for improving the code to have a more Mautic-like solution. Also, I was not sure which parts of the solution to test and how. Any suggestions?

Codecov Report

Attention: Patch coverage is 25.00000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 63.68%. Comparing base (47e8dee) to head (88f1c12).
Report is 11 commits behind head on 5.2.

@@             Coverage Diff              @@
##                5.2   #14888      +/-   ##
============================================
- Coverage     63.69%   63.68%   -0.01%     
- Complexity    34663    34676      +13     
============================================
  Files          2273     2274       +1     
  Lines        103710   103729      +19     
============================================
+ Hits          66054    66058       +4     
- Misses        37656    37671      +15     

In terms of design, I think changing the field type is the path for situations like these

Buttons are adequate, thank you for the fix

@andersonjeccel I was wondering if something like No change would be more descriptive than just x. What do you think?

@patrykgruszka sounds great!

@escopecz I have two issues with the code that I'm not able to solve:

First of all, I didn't manage to get the overall button responsive. So when the window gets to small it overlaps with other stuff

I tried a few different approaches but none of them worked. Do you have any best practice in your mind to solve the problem?

Number two is regarding my functions tests. After adding bool custom fields, I don't mange to set them to the desired value. Do you have an explanation for that?

 $lead       = $this->contactRepository->getEntity($id);

public function createBoolField(string $alias, string $label): void
{
    $field = new LeadField();
    $field->setAlias($alias);
    $field->setLabel($label);
    $field->setType('bool');
    $field->setIsVisible(true);
    $this->em->persist($field);
    $this->em->flush();
}

$this->createBoolField('bool1', 'Bool 1');
$lead->addUpdatedField('bool1', 1);

@JonasLudwig1998 I'm not sure about the responsiveness. @andersonjeccel may give a better advise here. I'd look into the columns and make them into a single column before the button group overflow. And the "x" was there for this reason too, I'd think.

For the test, you have to use the FieldModel to create a new field as it's doing also DB schema migrations. Check other tests how it's being done. For example the CampaignDecisionTest::makeField() method. You can extract that to a trait so we don't duplicate the field creation in so many places. But it doesn't matter if you do, it makes tests more readable if it's all in 1 file.

Another thing is that if an ALTER TABLE query is executed then the test cannot roll back. So make sure you turn that off with

protected $useCleanupRollback = false;

One more thing I'd recommend is to create a new contact with new Lead() instead of using API, getting ID and fetching it. It speeds up the test by a lot. I hope that's all that's needed to make the test work.

And please order the methods from public to protected to private so the public methods are at the top and private at the bottom of the classes.

This field type is stored in the database as boolean, so it should be:

$field->setType('boolean');

@JonasLudwig1998 the columns rendering each field, use classes "col-md-6 col-xs-12" (both together, will generate 2 columns on desktop and one above the other on mobile)

@andersonjeccel I changed _updatelead_action_widget.html.twig and _updatecompany_action_widget.html.twig to make the campaign action forms responsive (by adding col-md-6 col-xs-12").
Is this a good solution or is there a smarter and cleaner solution?

@escopecz Your suggestions are implemented now

@JonasLudwig1998 I'm not sure about the responsiveness. @andersonjeccel may give a better advise here. I'd look into the columns and make them into a single column before the button group overflow. And the "x" was there for this reason too, I'd think.

For the test, you have to use the FieldModel to create a new field as it's doing also DB schema migrations. Check other tests how it's being done. For example the CampaignDecisionTest::makeField() method. You can extract that to a trait so we don't duplicate the field creation in so many places. But it doesn't matter if you do, it makes tests more readable if it's all in 1 file.

Another thing is that if an ALTER TABLE query is executed then the test cannot roll back. So make sure you turn that off with

protected $useCleanupRollback = false;

One more thing I'd recommend is to create a new contact with new Lead() instead of using API, getting ID and fetching it. It speeds up the test by a lot. I hope that's all that's needed to make the test work.

And please order the methods from public to protected to private so the public methods are at the top and private at the bottom of the classes.

Note: I realised I had not tested the company boolean field updates in campaigns which I've just done - and this works too.