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

Filter empty keywords from keywords list on dataset view #2057

Closed
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## Unreleased

- Feat #2027: Filter empty keywords from keywords list on dataset view
- Feat #456: Improve DataCite metadata by migrating to Datacite version 4.6
- Fix #1727: Sort files and samples by id in descending order when querying

Expand Down
6 changes: 5 additions & 1 deletion protected/components/StoredDatasetMainSection.php
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,11 @@ public function getKeywords(): array
$flatten = function ($row) {
return $row['value'];
};
return array_map($flatten, $result);
$keywords = array_map($flatten, $result);
$filteredKeywords = array_filter($keywords, function ($keyword) {
return !empty(trim($keyword));
});
return $filteredKeywords;
}

/**
Expand Down
9 changes: 7 additions & 2 deletions protected/views/dataset/view.php
Original file line number Diff line number Diff line change
Expand Up @@ -92,10 +92,15 @@ function showText() {
</div>

<div class="subsection">
<?php if (!empty($mainSection->getKeywords())) { ?>
<?php
$validKeywords = array_filter(
$mainSection->getKeywords(), function($keyword) {
return strlen(trim($keyword)) > 0;
});
if (!empty($validKeywords)) { ?>
Comment on lines +95 to +100
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Does these changes necessary as there is already if (!empty($mainSection->getKeywords())) if the component class has been updated properly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kencho51 in my opinion it is fine to double check things in both client and server so I don't mind to leave it but also you have a point that both checks are serving the same purpose

As per your next comment, let's see how to proceed with this PR

<p>Keywords:</p>
<ul class="list-inline">
<? foreach ($mainSection->getKeywords() as $keyword_link) {
<? foreach ($validKeywords as $keyword_link) {
echo "<li>$keyword_link</li>";
}
?>
Expand Down
12 changes: 12 additions & 0 deletions tests/_support/Steps/CuratorSteps.php
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,18 @@ public function iFillInKeywordsFieldsWith($keyword)
$this->I->waitForText($keyword, 5, ".tag-editor-tag");
}

/**
* @When I remove all the keywords
*/
public function iRemoveAllKeywords()
{
$deleteButtons = $this->I->grabMultiple('.tag-editor .tag-editor-delete');
foreach ($deleteButtons as $button) {
$this->I->click('.tag-editor .tag-editor-delete');
$this->I->wait(1);
}
}

/**
* @Then I should see the application version
*/
Expand Down
8 changes: 8 additions & 0 deletions tests/acceptance/AdminDatasetForm.feature
Original file line number Diff line number Diff line change
Expand Up @@ -496,3 +496,11 @@ Feature: form to update dataset details
And I press the button "Next >"
And I wait "1" seconds
Then I should see "Parrot.k31.NetworkTest.txt"

@ok @issue-2027
Scenario: Keywords label not visible if keywords are removed from dataset
Given I am on "/adminDataset/update/id/200"
And I remove all the keywords
And I press the button "Save"
When I am on "/dataset/100142"
Then I should not see "Keywords:"
Comment on lines +500 to +506
Copy link
Contributor

Choose a reason for hiding this comment

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

issue: I found that this acceptance test is also passing when I checked out the develop branch, which means the existing code can already make sure no Keywords: if the dataset contains empty keywords.
thought: The issue may stem from the value column of some datasets containing special characters, eg, white spaces after the data ingestion from the excel spreadsheet.
**quick-work-around:" Log into the dataset admin page, and click save, the special chars will be removed as the update action will trigger replaceKeywordsForDatasetIdWithString() in the controller class to sanitize the keywords.
thought: So, I think this PR may not actually needed, or simply create a migration script to trim all the existing value column in the dataset_attributes table, @rija and @pli888 what do you think?