Update checkConfig class and to benefit configuration flows to use checkConfig #228
Conversation
Michael-Dratch
requested review from
Justin-MacIntosh
and removed request for
prestoncabe
Michael-Dratch
changed the title
Justin-MacIntosh
left a comment
Justin-MacIntosh
left a comment
There was a problem hiding this comment.
Only 2 actual "issues", but this is great work. Thank you for the effort here!
| return new ArrayList<>(latestVersionMap.values()); | ||
| } | ||
|
|
||
| private static int compareVersions(String v1, String v2) { |
There was a problem hiding this comment.
nit: I'd really love a file for util functions related to version numbers, but that can be a future change.
| return checks.slice().sort((check1, check2) => check2.version - check1.version); | ||
| return checks | ||
| .slice() | ||
| .sort((check1, check2) => check2.version - check1.version); |
There was a problem hiding this comment.
issue: We need another "normalized compare" here to properly sort these.
| if (!benefit) return; | ||
|
|
||
| const updatedChecks: CheckConfig[] = [...benefit.checks, newCheck] | ||
| console.log("updating benefit"); |
There was a problem hiding this comment.
nit: Remove logs if unneeded anymore.
| public class CheckConfig { | ||
| private String checkId; | ||
| private String checkName; | ||
| private String checkVersion; |
There was a problem hiding this comment.
issue: I'm wondering about module and whether that should be here too. We can already derive it from the checkId, but that's true of version as well, so maybe we don't even need version here as it's own param? We should try to decide if anything already in checkId is pulled from checkId, or if it's stored redundantly.
There was a problem hiding this comment.
I'll add module, I'm worried that storing data in the id itself was a bad idea and would like to rely more on the actual attributes
There was a problem hiding this comment.
For what it's worth I think the composite property was a cool idea, but it may make sense to make an Index in the Firestore Database for that purpose instead.
Updates so that the Benefit configuration pages use the benefit checkConfigs and not EligibilityCheck objects. Also included some naming changes to make names consistent across backend and frontend.