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

SPO-41: make SharedPrint::Phase3Validator support other_commitments #279

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

mwarin
Copy link
Contributor

@mwarin mwarin commented Jun 14, 2023

The (user facing) specs say you can submit commitments with the field other_commitments, and that field did exist, but the commitments loader had no code that knew how parse that field.

This PR adds capability to deal with other_commitments.

@coveralls
Copy link

coveralls commented Jun 28, 2023

Coverage Status

coverage: 95.064% (-0.07%) from 95.137% when pulling 9691794 on SPO-41 into 38a252b on main.

@mwarin mwarin marked this pull request as ready for review July 19, 2023 14:11
@mwarin
Copy link
Contributor Author

mwarin commented Jul 19, 2023

I have tested this in dev with the production file Tufts_print_retention_20230706.tsv and it works (given the proper setup). I'd like to release this asap so I can move a couple of operational tickets along.

@@ -85,5 +86,14 @@ def deprecation_validation
errors.add(:deprecation_date, "can't be set without a deprecation status.")
end
end

# If one of other_program/other_retention_date is set they must both be set
def other_commitment_validation
Copy link
Member

Choose a reason for hiding this comment

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

We have a couple of these fields that are linked together now (other commitment & deprecation) - it might be worth representing those at least in the code (if not in the actual data store) as their own object. Not a change we'd need to make now, but worth thinking about especially as we add additional reporting based on those fields.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. There is some creep towards that, and it should be monitored in case there's a smell.

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 is all clear enough to me. We are starting to see some issues emerging with data clumps within commitment that suggest we should start thinking about commitment as being composed of multiple objects, but that can certainly be a future change rather than something to do right now.

@mwarin mwarin merged commit 48cbd1d into main Jul 19, 2023
@mwarin mwarin deleted the SPO-41 branch July 19, 2023 15:21
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