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

Adds an option to export CSV with labels as heading #361

Open
wants to merge 4 commits into
base: 2.5
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions Controller/DynamicController.php
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<?php

Check failure on line 1 in Controller/DynamicController.php

View workflow job for this annotation

GitHub Actions / PHP Lint

Ignored error pattern #^Parameter \#2 \$locale of method Sulu\\Bundle\\FormBundle\\ListBuilder\\DynamicListFactory\:\:build\(\) expects string, mixed given\.$# in path /home/runner/work/SuluFormBundle/SuluFormBundle/Controller/DynamicController.php was not matched in reported errors.

/*
* This file is part of Sulu.
Expand Down Expand Up @@ -64,32 +64,40 @@
*/
private $viewHandler;

/**
* @var bool
*/
private $prefersLabeledExports;

public function __construct(
DynamicRepository $dynamicRepository,
DynamicListFactory $dynamicListFactory,
MediaManagerInterface $mediaManager,
EntityManager $entityManager,
FormRepository $formRepository,
ViewHandler $viewHandler
ViewHandler $viewHandler,
bool $prefersLabeledExports
) {
$this->dynamicRepository = $dynamicRepository;
$this->dynamicListFactory = $dynamicListFactory;
$this->mediaManager = $mediaManager;
$this->entityManager = $entityManager;
$this->formRepository = $formRepository;
$this->viewHandler = $viewHandler;
$this->prefersLabeledExports = $prefersLabeledExports;
}

/**
* Return dynamic form entries.
*/
public function cgetAction(Request $request): Response
{
$locale = $this->getLocale($request);
$filters = $this->getFilters($request);
$view = $this->getView($request);
$page = $request->get('page', 1);
$limit = $request->get('limit');
$offset = (int) (($page - 1) * $limit);
$view = $request->get('view', 'default');
$sortOrder = $request->get('sortOrder', 'asc');
$sortBy = $request->get('sortBy', 'created');

Expand All @@ -100,7 +108,7 @@
$offset
);

$entries = $this->dynamicListFactory->build($entries, $view);
$entries = $this->dynamicListFactory->build($entries, $locale, $view);

Check failure on line 111 in Controller/DynamicController.php

View workflow job for this annotation

GitHub Actions / PHP Lint

Parameter #2 $locale of method Sulu\Bundle\FormBundle\ListBuilder\DynamicListFactory::build() expects string, string|null given.

Choose a reason for hiding this comment

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

Interestingly, this is actually fixing a bug. The second argument for the build method in DynamicListFactoryInterface is the locale. But until now, the view was passed instead.

public function build(array $dynamics, string $locale, string $builder = 'default'): array;

The only reason it was working is because the third argument has a fallback and in the existing DynamicListBuilder the locale (which would be default) is not being used.

Choose a reason for hiding this comment

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

I just noticed #360 😄


// avoid total request when entries < limit
if (\count($entries) == $limit) {
Expand Down Expand Up @@ -185,4 +193,11 @@
{
return $request->get('locale', $request->getLocale());
}

public function getView(Request $request): string
{
$defaultView = ($this->prefersLabeledExports && $request->getRequestFormat() === 'csv') ? 'labeled' : 'default';

return $request->get('view', $defaultView);

Check failure on line 201 in Controller/DynamicController.php

View workflow job for this annotation

GitHub Actions / PHP Lint

Method Sulu\Bundle\FormBundle\Controller\DynamicController::getView() should return string but returns mixed.
}
}
4 changes: 4 additions & 0 deletions DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ public function getConfigTreeBuilder(): TreeBuilder
->arrayNode('dynamic_disabled_types')
->prototype('scalar')->end()->defaultValue([])
->end()
->booleanNode('dynamic_labeled_exports')
->info('Prefers labels instead of fields key as headings during export.')
->defaultFalse()
->end()
;

return $treeBuilder;
Expand Down
1 change: 1 addition & 0 deletions DependencyInjection/SuluFormExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ public function load(array $configs, ContainerBuilder $container): void
$container->setParameter('sulu_form.media_collection_strategy', $mediaCollectionStrategy);
$container->setParameter('sulu_form.static_forms', $config['static_forms']);
$container->setParameter('sulu_form.dynamic_disabled_types', $config['dynamic_disabled_types']);
$container->setParameter('sulu_form.dynamic_labeled_exports', $config['dynamic_labeled_exports']);

// Default Media Collection Strategy
$container->setAlias(
Expand Down
62 changes: 62 additions & 0 deletions ListBuilder/DynamicLabeledListBuilder.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
<?php

/*
* This file is part of Sulu.
*
* (c) Sulu GmbH
*
* This source file is subject to the MIT license that is bundled
* with this source code in the file LICENSE.
*/

namespace Sulu\Bundle\FormBundle\ListBuilder;

use Sulu\Bundle\FormBundle\Entity\Dynamic;
use Sulu\Bundle\FormBundle\Entity\Form;

class DynamicLabeledListBuilder extends DynamicListBuilder
{
private array $formFieldsLabelCache = [];

public function build(Dynamic $dynamic, string $locale): array
{
$entry = parent::build($dynamic, $locale);

if (empty($entry)) {
return $entry;
}

$labels = $this->getLabels($dynamic->getForm(), $locale);
$entryLabeled = [];

foreach ($entry[0] as $key => $value) {
$entryLabeled[$labels[$key] ?? $key] = $value;
}

return [$entryLabeled];
}

private function getLabels(Form $form, string $locale): array
{
if (!isset($this->formFieldsLabelCache[$form->getId()])) {
$labels = [];

foreach ($form->getFields() as $field) {
if (\in_array($field->getType(), Dynamic::$HIDDEN_TYPES)) {
continue;
}

$translation = $field->getTranslation($locale, false, true);

if ($translation) {
$label = $translation->getShortTitle() ?: \strip_tags($translation->getTitle());

Choose a reason for hiding this comment

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

I was testing this and I found an edge case: The "spacer" field does have a translation, but both getShortTitle and getTitle return null- in this case strip_tags will fail because it expects a string.

IMHO when there is no translation or all titles are null, I would just skip the loop instead of setting an empty string. Then in the build method above it will fall back to the key, which is better than nothing.

Choose a reason for hiding this comment

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

I found two more special fields that need different handling: freeText and headline.
Both are just there to add additional text to the form, but are no real fields. Before it appeared in the export as "freeText", "freeText1" with no content in the column etc. Now it's using the title which can potentially be a really long text. So either it should be skipped (it has no content anyway) or the key should be used instead.

Choose a reason for hiding this comment

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

There are some other special fields that I cannot test like recaptcha etc. which probably should be checked as well how it behaves with this new feature.

Copy link

@rogamoore rogamoore May 25, 2023

Choose a reason for hiding this comment

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

Looks like there is already a constant with these special fields: https://github.com/sulu/SuluFormBundle/blob/2.5/Entity/Dynamic.php#L36

And the getFields method has a hideHidden flag, which does exactly what we want - skip these fields 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, sorry for the delay. I've updated the PR with your suggestions.

$labels[$field->getKey()] = $label;
}
}

$this->formFieldsLabelCache[$form->getId()] = $labels;
}

return $this->formFieldsLabelCache[$form->getId()];
}
}
9 changes: 9 additions & 0 deletions Resources/config/services.xml
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,14 @@
<tag name="sulu_form.dynamic_list_builder" alias="simple" />
</service>

<service id="sulu_form.list_builder.dynamic_labeled_list_builder"
class="Sulu\Bundle\FormBundle\ListBuilder\DynamicLabeledListBuilder">
<argument>%sulu_form.dynamic_list_builder.delimiter%</argument>
<argument type="service" id="router" />

<tag name="sulu_form.dynamic_list_builder" alias="labeled" />
</service>

<!-- Repositories -->
<service id="sulu_form.repository.form" class="Sulu\Bundle\FormBundle\Repository\FormRepository">
<factory service="doctrine.orm.entity_manager" method="getRepository"/>
Expand Down Expand Up @@ -197,6 +205,7 @@
<argument type="service" id="doctrine.orm.entity_manager"/>
<argument type="service" id="sulu_form.repository.form"/>
<argument type="service" id="fos_rest.view_handler"/>
<argument>%sulu_form.dynamic_labeled_exports%</argument>
</service>

<!-- Form Controller -->
Expand Down
8 changes: 8 additions & 0 deletions Resources/doc/dynamic.md
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,14 @@ sulu_form:
protected: true
```

## Export to CSV with labels as heading
By default, forms are exported with the first row filled with the field keys. Enable the following option to fill it with field labels instead:

```yml
sulu_form:
dynamic_labeled_exports: true
```

## Test Checklist

The following things you should check when implement the dynamic form type on your website.
Expand Down
Loading