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

Fix CSV export issue #2215 #2216

Merged
merged 4 commits into from
Nov 27, 2024
Merged

Fix CSV export issue #2215 #2216

merged 4 commits into from
Nov 27, 2024

Conversation

zahardev
Copy link
Contributor

@zahardev zahardev commented Nov 22, 2024

💾 Build file (6603f79).

@zahardev zahardev force-pushed the 2215-csv-export-issue branch from 2338f59 to 4734287 Compare November 22, 2024 17:58
*/
$glue = apply_filters( 'gravityview/template/field/csv/glue', ';', $gravityview );
// It's the parent field, not an input
if ( ! $is_single_input ) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrcasual This is the same logic as in the field-checkbox-html.php file.

@@ -173,7 +177,8 @@ function ( $field ) use ( $allowed_field_ids ) {
// remove all links from output.
$field->update_configuration( [ 'show_as_link' => '0' ] );

$source = is_numeric( $field->ID ) ? $view->form : new \GV\Internal_Source();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrcasual This was the cause of the bug - it always used the main form as the source, even when the field belonged to the joined form.

@zahardev zahardev requested a review from mrcasual November 22, 2024 18:08
@mrcasual mrcasual requested a review from Mwalek November 22, 2024 18:14
@mrcasual
Copy link
Collaborator

@Mwalek, please test.

@Mwalek
Copy link

Mwalek commented Nov 25, 2024

@mrcasual @zahardev the check marks no longer show as strings, but the CSV output is still not perfect.

If you look at the row of "Mutale" in the screenshot below, you'll see that the check mark should be present on Nigeria and Sweden.

Screenshot 2024-11-25 at 3 15 54 PM

However, when the CSV is exported, the check mark is present on Nigeria and Portugal, this is wrong.

Screenshot 2024-11-25 at 3 15 25 PM

If you want to export any of my test data: https://test-notifs.try.gravitykit.com/wp-admin/post.php?post=19&action=edit (test:test)

* @param GF_Field_Checkbox $field GravityView field.
* @param Template_Context $gravityview The context.
*/
$output = apply_filters( 'gravityview/template/field/csv/tick', '✓', $entry, $field, $gravityview );
Copy link
Collaborator

Choose a reason for hiding this comment

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

@zahardev, is there a reason why you didn't use the existing gravityview_field_tick filter?

Copy link
Contributor Author

@zahardev zahardev Nov 25, 2024

Choose a reason for hiding this comment

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

@mrcasual The gravityview_field_tick filter is used on web pages, and its default value is <span class="dashicons dashicons-yes"></span>. We can't use this value in CSV exports as it won't convert to a tick symbol. That's why I added a new filter specifically for CSV handling. Please let me know if we can somehow improve it or use gravityview_field_tick instead.

Copy link
Contributor

@rafaehlers rafaehlers Nov 25, 2024

Choose a reason for hiding this comment

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

Thanks for the clarification @zahardev! @mrcasual that explains why DataTables wasn't able to export the ticks (DT strips HTML), and I had to use an emoji in place: https://github.com/GravityKit/DataTables/issues/60#issuecomment-856311493

@zahardev
Copy link
Contributor Author

@Mwalek

Just tried to check the issue on https://test-notifs.try.gravitykit.com, and it works correctly for me.

2024-11-25-02ad
2024-11-25-cf40

Could you please try reproducing the issue once again?

@Mwalek
Copy link

Mwalek commented Nov 26, 2024

Hey @zahardev, I've narrowed down the issue. It only occurs when the download is initiated by adding /csv/ to the View URL. E.g:

https://test-notifs.try.gravitykit.com/view/combi-1/csv/

Please let me know if you can reproduce the issue when the URL above is used.

@zahardev
Copy link
Contributor Author

Hey @Mwalek, thank you, I've managed to reproduce it locally.

@zahardev
Copy link
Contributor Author

@Mwalek I've made a fix for it, could you please check if it works for you now?

@Mwalek
Copy link

Mwalek commented Nov 27, 2024

@zahardev thank you, it works perfectly now!

cc. @mrcasual

@mrcasual mrcasual removed the Untested label Nov 27, 2024
@mrcasual mrcasual merged commit b3a6650 into develop Nov 27, 2024
1 check passed
@mrcasual mrcasual deleted the 2215-csv-export-issue branch November 27, 2024 15:28
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.

4 participants