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

Fixing Required Attachments #419

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

GradyD
Copy link

@GradyD GradyD commented Jun 11, 2020

Description

closes #336

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have added Pester Tests that describe what my changes should do.
  • I have updated the documentation accordingly.

@GradyD GradyD requested review from a team as code owners June 11, 2020 15:30
Copy link
Member

@lipkau lipkau left a comment

Choose a reason for hiding this comment

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

I added a few inline comments in your code.
Additionally, please consider:

  • I didn't test it, but I am reasonably confident, that the 400 response contains the information why the request failed (missing required fields). Why can't we use this information for our output? Adding our own business logic for identifying why a request failed should be avoided as long as the information returned by the API is not good enough. As I don't know how the response looks like (without looking it up), please state your case why you deem necessary to add this logic to the module.
  • I have an improvement for the error handling staged on my local machine to improve it a bit. take a look if your code could benefit from it (I didn't check):
        $allErrors = ($responseObject.errorMessages + $responseObject.message + $responseObject.errors)

                if ($allErrors.Count -eq 0) {
                    throw "Unable to handle error"
                }

                foreach ($_error in $allErrors) {
                    if ($_error.Count -gt 0) {
                        $writeErrorSplat = @{
                            Exception    = $exception
                            ErrorId      = $errorId
                            Category     = $errorCategory
                            Message      = Out-String -InputObject $_error
                            TargetObject = $targetObject
                            Cmdlet       = $Cmdlet
                        }
                        WriteError @writeErrorSplat
                    }
                }
  • You must still update the existing tests to pass with these changes
  • You should add new tests (with a mocked 400 response) showing how these errors are handled


if ($errorArray.count -gt 0) {
$errorParameter = @{
ExceptionType = "System.Net.Http.HttpRequestException"
Copy link
Member

Choose a reason for hiding this comment

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

Please use the same exception type as Invoke-WebRequest returns
(System.Net.WebException for PSVersion -lt 6 and Microsoft.PowerShell.Commands.HttpResponseException for PSVersion -ge 6)

@@ -44,7 +44,29 @@ function Resolve-ErrorWebResponse {
try {
$responseObject = ConvertFrom-Json -InputObject $responseBody -ErrorAction Stop

$errorArray = @()
Copy link
Member

Choose a reason for hiding this comment

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

Please use [System.Collections.ArrayList]

Reading material on why: https://gist.github.com/kevinblumenfeld/4a698dbc90272a336ed9367b11d91f1c

foreach ($_error in ($responseObject.errorMessages + $responseObject.errors)) {

foreach ($_err in $_error.PSObject.Properties.Value) {
if ($_err -like "*You must specify a summary of the issue." -Or $_err -like "*is required.") {
Copy link
Member

Choose a reason for hiding this comment

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

I don't trust parsing text from the answer. What happens when the system is installed in another language?
(Unfortunately I can't test that)

Is there any other way we can identify this?

Category = "AuthenticationError"
Cmdlet = $Cmdlet
}
ThrowError @errorParameter
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't use throw here. Throw will cause the entire script to fail when this happens (unless used with try {} catch {}.
Please use WriteError (can have the same behavior if wanted, by adding -ErrorAction Stop to the script)

@lipkau lipkau added this to To Do in Improvements via automation Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Improvements
  
To Do
Development

Successfully merging this pull request may close these issues.

Unable to create a ticket with attachment New-JiraIssue
2 participants