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

Conversation

dev-newvisibility
Copy link
Contributor

Q A
New feature? yes
Related issues/PRs #359
License MIT

What's in this PR?

When exporting forms to CSV, the first row is filled with the field keys. This may be the preferable option if the CSV is to be read by a machine. However, if the CSV is to be processed by a human, a label is more helpful to distinguish the columns.
This PR adds an option to the sulu_form configuration to choose between the two formats.

With the option disabled (same as before):
nok

With the option enabled:
ok

@@ -100,7 +108,7 @@ public function cgetAction(Request $request): Response
$offset
);

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

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 😄


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

Choose a reason for hiding this comment

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

Should be $this->formFieldsLabelCache I guess :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thank you!

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

@alexander-schranz
Copy link
Member

I'm not sure if a config option is the best here 🤔

We should have a general discussion about the CSV Export and Labels as sure FormBundle is one place but there are not of other CSV Exports which currently only output keys.

What I'm not sure if a config options is the right way here or if we should get that generally into the UI as an option. The keys make sense in case of exports done for imports or external systems, but are in case of the form bundle not always user friendly (e.g. checkbox, checkbox_1, ...).

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

Successfully merging this pull request may close these issues.

3 participants