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-4287 Make harvest_run id not a primary key. #4346

Draft
wants to merge 5 commits into
base: 2.x
Choose a base branch
from

Conversation

swirtSJW
Copy link
Contributor

@swirtSJW swirtSJW commented Nov 20, 2024

Fixes #4287

Describe your changes

QA Steps

This list is INCOMPLETE at this time.

From existing data

  • git checkout 2.18.3 (the tag before runs became an entity)
  • init the site
  • ddev drush dkan:sample-content:create
  • ddev drush cron
    [] validate no errors show in terminal from these commands.

From new data

[] validate that the Run table list displays ___

  • Add manual QA steps in checklist format for a reviewer to perform. Be as specific as possible, provide examples if appropriate.

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-4287-de-key-harvest-run branch 2 times, most recently from 8ce1554 to 59ea0a1 Compare November 20, 2024 22:34
@swirtSJW swirtSJW self-assigned this Dec 4, 2024
@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch 9 times, most recently from 9a08467 to f1d22db Compare December 10, 2024 21:16
*
* @return \Drupal\harvest\HarvestRunInterface|\Drupal\Core\Entity\EntityInterface|null
* The loaded entity or NULL if none could be loaded.
*
* @deprecated in dkan:2.19.11 and is removed from dkan:3.0.0 Use HarvestService::load().
Copy link
Contributor Author

@swirtSJW swirtSJW Dec 10, 2024

Choose a reason for hiding this comment

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

It is possible that this can't be deprecated. Everywhere it is still called, there is no access to the actual ID. Mainly in tests where it is pulling previous specific run instances.

@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch 4 times, most recently from b44beae to d87e4fa Compare December 11, 2024 04:32
@swirtSJW
Copy link
Contributor Author

There are still two test failures that are not immediately clear to me why they are failing
image

@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch 2 times, most recently from c5c9048 to e3e46fa Compare December 11, 2024 14:09
@swirtSJW
Copy link
Contributor Author

Got it down to just one test failure
testGetExtractedUuids
Failed asserting that two arrays are equal.

@dafeder
Copy link
Member

dafeder commented Dec 11, 2024

@swirtSJW I don't see any test failures at the moment?

@swirtSJW
Copy link
Contributor Author

@paul-m Can you take a first pass at code review on this?
Tests are passing but I will hold off on adding QA steps until I perform this whole upgrade on PDC. Right now this is successful on vanilla but has not been fully run on an existing site.

@swirtSJW swirtSJW requested a review from paul-m December 11, 2024 22:56
@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch from e3e46fa to b91e610 Compare December 11, 2024 23:06
Copy link
Contributor

@paul-m paul-m left a comment

Choose a reason for hiding this comment

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

Under the 2.x branch, I used sample_content to generate a harvest to run (drush dkan:sample-content:create), and then ran it (using cron).
Then I switched to this branch and ran drush updb.
Everything seemed to work... I did notice that the one harvest run in my harvest_runs table didn't have a UUID.

So I went ahead and did a new installation with this branch, and then ran the sample content harvest, and it did have a UUID.

So perhaps the update process doesn't generate UUIDs the way just creating an entity does...

Also I'd suggest that the utility functions, such as harvest_get_temp_run_ids() in harvest.install should be in HarvestUtility. That way it's all in one place and labeled as part of the update process.

@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch 2 times, most recently from 68ddecb to d07fb38 Compare December 12, 2024 19:10
@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch 2 times, most recently from 5cb3055 to 2407716 Compare December 12, 2024 23:03
@swirtSJW
Copy link
Contributor Author

Everything seemed to work... I did notice that the one harvest run in my harvest_runs table didn't have a UUID

So it looks like a simple database write did not kick off the magic that would add a uuid that a normal drupal save would. So I intentionally added it.

@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch 3 times, most recently from ed4ea52 to f6a4484 Compare December 13, 2024 02:53
@swirtSJW swirtSJW force-pushed the DKAN-4287-de-key-harvest-run branch from f6a4484 to e6d855f Compare December 13, 2024 22:33
@swirtSJW
Copy link
Contributor Author

I am stumped by this test failure.

There was 1 error:

1) Drupal\Tests\datastore\Unit\Plugin\QueueWorker\ImportJobTest::testBasics
TypeError: Drupal\datastore\Plugin\QueueWorker\ImportJob::getParser(): Return value must be of type Contracts\ParserInterface, CsvParser\Parser\Csv returned

It seems completely unrelated to the code in this PR.

Beyond this test @paul-m I made all the changes you requested.

@paul-m
Copy link
Contributor

paul-m commented Dec 19, 2024

Over in #4177 we deprecated some interfaces, which led to the fails.

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.

Converting runs from old harvest_ID_runs to harvest_runs fails if duplicate time stamps
3 participants