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.

Sign up for GitHub

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

Jump to bottom

Get all members at once, rather than one at a time from the db #310

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Get all members at once, rather than one at a time from the db #310

merged 1 commit into from
Feb 13, 2025

Conversation

hackartisan
Copy link
Member

closes #227

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 13, 2025 •
edited
Loading

Container Scanning Status: ✅ Success


ghcr.io/pulibrary/dpul-collections:pr-310 (debian 12.6)
=======================================================
Total: 0 (HIGH: 0, CRITICAL: 0)

@hackartisan hackartisan marked this pull request as ready for review February 13, 2025 17:32
tpendragon
tpendragon approved these changes Feb 13, 2025
View reviewed changes
test/dpul_collections/indexing_pipeline_test.exs
@@ -99,6 +99,19 @@ defmodule DpulCollections.IndexingPipelineTest do
assert IndexingPipeline.get_figgy_resource!(ephemera_folder_id).id == ephemera_folder_id
end

test "get_figgy_resource!/1 errors when the resource isn't there" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Where'd this test come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added this, it seemed like the error case wasn't tested and then I used it as a model to check the behavior of the new function. Do you think it's redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not at all, it just seemed unrelated so I was curious if there was something in developing this that brought a bug to light.

@tpendragon
Copy link
Contributor

Just one question, but approved - feel free to merge.

@tpendragon tpendragon merged commit 44eb205 into main Feb 13, 2025
4 checks passed
@tpendragon tpendragon deleted the 227-members-query branch February 13, 2025 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpendragon tpendragon tpendragon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Member hydration should use a members query instead of querying for every resource
2 participants
@hackartisan @tpendragon