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

DKAN-4291 add mysql empty row removal option #4292

Merged
merged 4 commits into from
Oct 21, 2024

Conversation

swirtSJW
Copy link
Contributor

@swirtSJW swirtSJW commented Sep 17, 2024

Fixes #4291

Describe your changes

  • adds a settings page for the datastore_mysql_importer to allow for automatic removal of empty rows after import.
  • Adds two tests, one for the removal option enabled and one for it disabled.

QA Steps

  • Login as admin
  • Visit /admin/dkan/datastore/mysql_import
  • Validate wording and that changes to the one setting, are properly saved.
    image
  • Validate the menu item placement for the settings page
    image
  • in terminal run ddev dkan-phpunit --group datastore_mysql_import
  • validate that tests pass.

Checklist before requesting review

If any of these are left unchecked, please provide an explanation

  • I have updated or added tests to cover my code
  • I have updated or added documentation

@swirtSJW swirtSJW force-pushed the DKAN-4291-add-mysql-empty-rows-option branch 2 times, most recently from 5e7ec8f to 01f1300 Compare September 17, 2024 21:45
@swirtSJW swirtSJW self-assigned this Sep 18, 2024
@janette
Copy link
Member

janette commented Sep 18, 2024

Thanks for working on this @swirtSJW please include tests for the new code.

@dmundra
Copy link
Contributor

dmundra commented Sep 20, 2024

Thanks for working on this @swirtSJW please include tests for the new code.

@swirtSJW check out the tests modules/datastore/modules/datastore_mysql_import/tests/src/Kernel/Service/MysqlImportTest.php in like https://github.com/GetDKAN/dkan/pull/4293/files

@swirtSJW swirtSJW force-pushed the DKAN-4291-add-mysql-empty-rows-option branch from 99f0f61 to 6e4757f Compare October 14, 2024 18:07
@swirtSJW swirtSJW force-pushed the DKAN-4291-add-mysql-empty-rows-option branch from 8e9b6a1 to a501f86 Compare October 16, 2024 06:36
@swirtSJW swirtSJW force-pushed the DKAN-4291-add-mysql-empty-rows-option branch from a501f86 to c7fc297 Compare October 16, 2024 06:40
@swirtSJW swirtSJW marked this pull request as ready for review October 16, 2024 06:53
@swirtSJW swirtSJW requested a review from janette October 16, 2024 06:54
Copy link
Member

@janette janette left a comment

Choose a reason for hiding this comment

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

Minor tweaks please

@@ -7,6 +7,6 @@ To use, simply enable _datastore_mysql_import_ and clear your cache.

## Differences in behavior with the default DKAN importer.

* Any "blank" rows in your data file, including carriage returns, will be imported into the datastore as empty rows. The default importer will ignore these rows.
* Any "blank" rows in your data file, including carriage returns, will be imported into the datastore as empty rows. The default importer ignores any blank rows. To have the Mysql Importer behave the same as default importer and ignore empty rows, enable the option on this page /admin/dkan/datastore/mysql_import
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a "the" before default importer in the second sentence?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Corrected.

mapping:
remove_empty_rows:
type: boolean
label: 'Remove empty rows in dataset'
Copy link
Member

Choose a reason for hiding this comment

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

More accurate to say 'Remove empty rows from the datastore table'

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

@@ -0,0 +1,6 @@
datastore_mysql_import.settings_form:
title: MySQl Import settings
Copy link
Member

Choose a reason for hiding this comment

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

MySQL with capital L

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

Copy link
Contributor Author

@swirtSJW swirtSJW left a comment

Choose a reason for hiding this comment

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

Requested changes made.

@swirtSJW swirtSJW requested a review from janette October 21, 2024 15:34
@janette janette merged commit b33a0e4 into 2.x Oct 21, 2024
11 checks passed
@janette janette deleted the DKAN-4291-add-mysql-empty-rows-option branch October 21, 2024 21:35
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.

Add option for Mysql Import to support removing empty rows
3 participants