-
Notifications
You must be signed in to change notification settings - Fork 35
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
B 20939 int #13538
Merged
Merged
B 20939 int #13538
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
eb7e10d
Feature working, others WIP
antgmann 9d6f335
Working test, looking for better date solution
antgmann 455cd2f
Better test solution
antgmann 6bfd43a
Merge branch 'B-20939-SSW-Prep-Dates' into B-20939-INT
antgmann 2fca7c0
Add timenow for AOA
antgmann 386be52
Merge branch 'B-20939-SSW-Prep-Dates' into B-20939-INT
antgmann 0499582
Merge branch 'integrationTesting' into B-20939-INT
antgmann 79e4807
remove erroneous comment
antgmann c7b25ae
Merge branch 'B-20939-SSW-Prep-Dates' into B-20939-INT
antgmann 180da9a
Add error handling
antgmann bd26759
Merge branch 'B-20939-SSW-Prep-Dates' into B-20939-INT
antgmann 0bb4ddf
Merge branch 'integrationTesting' into B-20939-INT
antgmann 1c19b76
Merge branch 'integrationTesting' into B-20939-INT
antgmann File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Binary file not shown.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lines 418 and 434: I'm assuming this is a worse case scenario? It should never get to this, right? You may want to throw an error and/or log here.
In addition, you want to create a generic method in place of formatAOADate/formatSSWDate methods that takes in the cert array, uuid and enum. It should return null if nothing matches. From there you can log or throw an error in the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
418 is what we discussed in the standup this morning. Right now, there's a way to download the AOA before the SC submits move details where their signature is added. Until we can revisit the aoa download availability, our PO just wants it to use the date the PDF generation was ran.
As for 434, this should never occur, so an error may be more appropriate here. If that's what we want to go with, I'll add it to the todo list for later today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone ahead and added the error handling to this method. Now, if there's ever an uncertified payment packet downloaded before it should be, it will error out. Honestly, making this sort of thing easier in this service was long overdue, so I think that's a great change.
As for the generic method idea, I'm going to push back against that because that's the solution I attempted initially. Due to the implementation in the way that the service determines which packet is being generated, as well as the best way for handling adding the dates themselves from the certs, and the two alternate solutions to when a date shouldn't be used... I believe the current solution is easier to read, easier to maintain, is marginally better performance-wise, and uses about the same amount of code as combining them.
I do realize that the amount of methods in this service is... kind of a lot, so I'd see why we'd want to cut down on them. But if I need more justification still, then an additional reason for adding 2 in this story is that these changes also removed a near-duplicate method in formatSignatureDate.
Just let me know if any objections.