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 importers sorting for last run and next run #977

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

kirkkwang
Copy link
Contributor

@kirkkwang kirkkwang commented Sep 16, 2024

Summary

🐛 Fix importers sorting for last run and next run

7d985dd

This commit will fix the sorting error that happens when the user is on
the importers index and attempts to sort by the last run or next run. We
add some migrations to add fields that the datatables can use so it can
sort properly. Previously, this was not working because the
last_imported_at and next_import_at fields were actually methods on the
importer_run object and not the importer object. We are adding a few
callbacks to the importer and importer_run models to ensure that the
fields are properly set when they are called from either the web or the
worker.

Ref:

Zight.Recording.2024-09-16.at.16.43.37.mp4

🤖 Update upload-artifact and download-artifact

aca33e5

CI was getting errors because the versions we were previously using were
deprecated. This commit updates the actions to the latest versions.

🐛 Remove Downloadable Files sorting

d4d4749

The downloadable files sorting was broken plus, it's not clear now a
downloadable file should be sorted.

⚙️ Add guard for new migrations

7011d72

This commit will add a guard to the new migrations to ensure that they
do not run if the columns already exist in the database.

Note

To get the changes all the Importers need to be updated, something like:

Importers.all.each(&:save)

This commit will fix the sorting error that happens when the user is on
the importers index and attempts to sort by the last run or next run. We
add some migrations to add fields that the datatables can use so it can
sort properly. Previously, this was not working because the
last_imported_at and next_import_at fields were actually methods on the
importer_run object and not the importer object.  We are adding a few
callbacks to the importer and importer_run models to ensure that the
fields are properly set when they are called from either the web or the
worker.

Ref:
- #956
@kirkkwang kirkkwang added patch-ver for release notes bug-fix labels Sep 16, 2024
CI was getting errors because the versions we were previously using were
deprecated.  This commit updates the actions to the latest versions.
@ShanaLMoore
Copy link
Contributor

Exporters were also reported with the same issue. Is there an exporter table we need to add a migration to as well?

The downloadable files sorting was broken plus, it's not clear now a
downloadable file should be sorted.
@kirkkwang
Copy link
Contributor Author

@ShanaLMoore thanks for checking on that. The only issue I saw was the downloadable files sorting, it was broken, but I'm just taking it out since no one was using it. It's not clear to me how to sort that column anyways. Number of files? Date? etc. If there's demand for this feature then I think we can figure something out later.

@ShanaLMoore
Copy link
Contributor

ShanaLMoore commented Sep 17, 2024

Sounds good. My other question is for existing application data. In addition to the migration, how are we handling existing importers to populate those fields? And would we need to for the sort to work properly? Perhaps a rake task is needed?

@kirkkwang
Copy link
Contributor Author

@ShanaLMoore We just need to update all the Importers and the changes will get applied.

@ShanaLMoore
Copy link
Contributor

right, so maybe we should add a rake task to the migration to accomplish this? vs relying on the dev to remember to do.

@kirkkwang
Copy link
Contributor Author

@ShanaLMoore ah okay, that's a good idea. So how would that look like? The dev has to run migrations, and then migration automatically runs the rake task?

@ShanaLMoore
Copy link
Contributor

ShanaLMoore commented Sep 18, 2024

Yes, I'll try to find an example. LaRita has set this up before somewhere.

update: scratch the above approach. After discussion we will...

Add a rake task to re save all importers and/or add the command to release notes. cc @kirkkwang

This commit will add a guard to the new migrations to ensure that they
do not run if the columns already exist in the database.
@ShanaLMoore ShanaLMoore force-pushed the i956-fix-sorting-on-last-run-and-next-run branch 2 times, most recently from 9fdc300 to 2a54cee Compare September 18, 2024 17:18
@kirkkwang kirkkwang force-pushed the i956-fix-sorting-on-last-run-and-next-run branch from 2a54cee to 4c2ce7c Compare September 18, 2024 17:35
This rake task will allow users to re save all their importers.  It
accounts for tenants if it is a Hyku application.

```sh
bundle exec rake bulkrax:resave_importers
```
@kirkkwang kirkkwang force-pushed the i956-fix-sorting-on-last-run-and-next-run branch from 4c2ce7c to 8db0a93 Compare September 18, 2024 18:31
@kirkkwang kirkkwang merged commit 8f6dd70 into main Sep 18, 2024
6 checks passed
@kirkkwang kirkkwang deleted the i956-fix-sorting-on-last-run-and-next-run branch September 18, 2024 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix patch-ver for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants