-
Notifications
You must be signed in to change notification settings - Fork 1
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
Pivotal ID # 183126684: Log Submission Draft ID #648
Pivotal ID # 183126684: Log Submission Draft ID #648
Conversation
- Include draft related information in the logs - Change the logging level for the retry web template from error to warning
@@ -65,16 +65,26 @@ class SubmissionSubmitter( | |||
try { | |||
logger.info { "${rqt.accNo} ${rqt.owner} Started processing submission request" } | |||
|
|||
rqt.draftKey?.let { draftService.setProcessingStatus(rqt.owner, it) } | |||
rqt.draftKey?.let { |
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 think this is adding too much complexity to this method, we times the same conditional (if draftkey != null) in this few lines. Not really sure about but we may consider include accNo and owner in draftService
operations to log on each status change. Not much better as we will be including parameters only for logging purposes but as it is righ it looks super verbose.
val processed = submissionProcessor.processSubmission(rqt) | ||
parentInfoService.executeCollectionValidators(processed) | ||
rqt.draftKey?.let { draftService.setAcceptedStatus(it) } | ||
rqt.draftKey?.let { | ||
logger.info { "${rqt.accNo} ${rqt.owner} Assigned accNo '${processed.accNo}' to draft with key '$it'" } |
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.
this one I can guess can be moved to submissionProcessor.processSubmission
where actuallty this happens (I am looking for way to reduce verbosity here )
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.
LGTM please consider comments
https://www.pivotaltracker.com/story/show/183126684