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

Clusterable Shared Print Commitments and loader #146

Merged
merged 4 commits into from
Dec 1, 2021

Conversation

jsteverman
Copy link
Contributor

Doesn't address updates or deprecations, or even identity (there are a lot of duplicates in the existing database).

My only concern before loading is the setting of cluster.last_modified when adding commitments, which seems unnecessary.

@jsteverman jsteverman requested review from aelkiss and mwarin November 19, 2021 15:00
@aelkiss
Copy link
Member

aelkiss commented Nov 19, 2021

Agreed, we should not update cluster.last_modified.

Copy link
Member

@aelkiss aelkiss left a comment

Choose a reason for hiding this comment

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

This all looks good to me and in-line with what we're doing with other data types. I think we can go ahead and merge once the last_updated thing is taken care of. It would be useful to have feedback from @mwarin before going too much farther, but I think we can proceed for now.


RSpec.describe Loader::SharedPrintLoader do
let(:json) do
'{"uuid":"e78047da-1193-43c7-aab0-492067d13f9c","organization":"ucsd","ocn":2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Break out and put in spec/fixtures/ ?

Copy link
Contributor

@mwarin mwarin left a comment

Choose a reason for hiding this comment

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

A few comments but nothing that would prevent this PR to be merged.

@jsteverman jsteverman force-pushed the HT-3228-sharedprint-class branch from 8e480ba to 8d99eff Compare November 30, 2021 17:35
@mwarin mwarin merged commit 19f2733 into main Dec 1, 2021
@aelkiss aelkiss deleted the HT-3228-sharedprint-class branch February 8, 2022 14:59
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.

3 participants