Move the harvest library into the harvest module #4427
Conversation
f12acb1 to
6927db5
Compare
fed08ec to
9c70e0a
Compare
There was a problem hiding this comment.
This looks good but I'm pretty sure that an upgrade would break existing harvests that are referring to classes in the Harvest\ETL namespace. So I think we need an update hook that will fix those. I'm working on steps to demonstrate the issue. We should probably do an upgrade test for this as well.
|
@beeyayjay to add update test |
| $harvest_service = \Drupal::service('dkan.harvest.service'); | ||
| try { | ||
| $result = $harvest_service->runHarvest('sample_content'); | ||
| } | ||
| catch (\Exception $exception) { | ||
| $result = $exception->getMessage(); | ||
| } | ||
| $this->assertSame('No items found to extract, review your harvest plan.', $result); | ||
|
|
||
| // Run update and recreate harvest service with updated data. | ||
| $this->runUpdates(); | ||
| $harvest_service = \Drupal::service('dkan.harvest.service'); | ||
|
|
||
|
|
||
| try { | ||
| $result = $harvest_service->runHarvest('sample_content'); | ||
| } | ||
| catch (\Exception $exception) { | ||
| $result = $exception->getMessage(); | ||
| } | ||
| $this->assertIsArray($result); | ||
| $this->assertSame('SUCCESS', $result['status']['extract']); |
There was a problem hiding this comment.
@beeyayjay it's confusing to have try/catches here, because it's not clear when you read the test which one is happening. Is it throwing an exception in each case? If so, let's not set $result in the try, because it's not clear which one is in the assertion. If it's not throwing an exception, we don't need a try/catch at all.
Also not sure what this test shows, maybe I'm missing something. Why does the harvest have different results before and after the update? Aren't we trying to demonstrate that it works the same?
There was a problem hiding this comment.
@dafeder we're trying to demonstrate that the update works as intended. runHarvest() fails before the update because the extractor namespace in the db is outdated. It should succeed after the update is applied. (Note that the second assignment of $harvest_service on L54 is required to load the updated data.)
I think I can replace the first try/catch with an expectException() and leave the second catch blank. The second runHarvest() shouldn't fail, but that's really what we're testing, and a failed assertion seems preferable to an uncaught exception if the test fails.
There was a problem hiding this comment.
I had to leave the first try/catch block in. expectException() worked, but the rest of the test didn't run.
I rewrote it a bit though, so hopefully it's clearer now.
There was a problem hiding this comment.
OK sorry re-reading this the logic of the test does make sense. But I'm still failing to see why the second try/catch is necessary. Removing the $result = does make the first more readable and the test more valid, thanks. See my suggestion below.
15875aa to
a7373e6
Compare
There was a problem hiding this comment.
@beeyayjay sorry almost there, see my last comment on the test please
modules/harvest/tests/src/Functional/HarvestCodeConsolidationTest.php
Outdated
Show resolved
Hide resolved
…est.php Co-authored-by: Dan Feder <dan.feder@civicactions.com>
Fixes #4373
see also 24373
Describe your changes
Moved code from harvest library (GetDKAN/harvest) into harvest module (GetDKAN/dkan/modules/harvest), and removed harvest library from DKAN build.
QA Steps
ddev drush dkan:harvest:list,ddev drush dkan:harvest:info sample-content,ddev drush dkan:harvest:revert sample-content,ddev drush dkan:harvest:run sample-content,ddev drush dkan:harvest:archive sample-content,ddev drush dkan:harvest:archive sample-content)Checklist before requesting review
If any of these are left unchecked, please provide an explanation