Implement removal of Jira issues via Remove-JiraIssue #265

hmmwhatsthisdo · 2018-05-25T22:05:17Z

Description

This PR adds Remove-JiraIssue, which will DELETE /api/2/issue/{issueIdOrKey} to remove an issue from Jira.

This PR resolves #264.

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.

hmmwhatsthisdo · 2018-05-25T22:14:14Z

I haven't yet implemented Pester tests for Remove-JiraUser as I wanted to gather feedback on what to do in the event an issue with sub-tasks is removed.

The Jira API includes a deleteSubtasks parameter on DELETE /api/2/issue/{issueIdOrKey}. According to the API documentation:

If the issue has subtasks you must set the parameter deleteSubtasks=true to delete the issue. You cannot delete an issue without its subtasks also being deleted.

Moreover, deleteSubtasks has no effect if the issue doesn't have subtasks:

If the issue has no subtasks this parameter is ignored. If the issue has subtasks and this parameter is missing or false, then the issue will not be deleted and an error will be returned.

I see there being three different options for handling this within JiraPS:

Expose deleteSubtasks as a parameter (this is the current functionality) and simply percolate the API error back to the user if they try to delete an issue when it would be required otherwise.
Always set deleteSubtasks to true. (This feels dangerous.)
Retrieve tasks (if passed by ID/Key/etc.) and check if the subtasks property is non-empty, then warn the user (possibly using $PSCmdlet.ShouldContinue() and act accordingly.

lipkau

1.) regarding your concern with sub-tasks:
The first suggestion of yours

Expose deleteSubtasks as a parameter (this is the current functionality) and simply percolate the API error back to the user if they try to delete an issue when it would be required otherwise.

IMHO it is the best suiting one for the module.

2.) About -Issue
I have some functions stashed which no longer take [Object] as input for -Issue.
This has the consequence of -Issue always to depend on the output of Get-JiraIssue.
But this will happen with the next major version anyway.
If you are interested in making this function only take [JiraPS.Issue] as input, let me know and I will share the code.
--> this has the advantage of not needing Resolve-JiraIssue and using

$parameter = @{
                URI        = $Issue.RestURL
                Method = "DELETE"

Eg: https://github.com/AtlassianPS/JiraPS/pull/266/files#diff-eb644f77aa0b0f42e9317e245a236817

3.) About Pester Tests
This might be a good place to start:
https://github.com/AtlassianPS/JiraPS/pull/266/files#diff-bd4e363afdce9b0c34fac95d8fad0c8f

lipkau · 2018-05-26T09:50:36Z

JiraPS/Public/Remove-JiraIssue.ps1

+            }
+
+            $parameter = @{
+                URI        = $resourceURi -f $issueObj.Key,$IncludeSubTasks


I don't believe this will work, as {1} has to be a string true or false
Did you test this?

Minimum reproducible test-case:

function Invoke-SwitchStringTest { Param( [switch]$TestSwitch ) Process { Return "Switch Output: {0}" -f $TestSwitch } } Invoke-SwitchStringTest -TestSwitch # Switch Output: True Invoke-SwitchStringTest # Switch Output: False

As the deleteSubTasks param is passed on the query-string, it being a string vs. a bool shouldn't have much of a bearing (the value isn't being quoted or similar).
I have already tested the API accepting True/False instead of true/false using the Atlassian REST API Browser, and my initial testing with Remove-JiraIssue itself seemed to indicate that the cmdlet functioned as expected.

lipkau · 2018-05-26T09:51:58Z

JiraPS/Public/Remove-JiraIssue.ps1

+                Credential = $Credential
+            }
+
+            $TargetText = "[$($issueObj.Key)] $($issueObj.Summary)"


is the same as $issueObj.ToString().
You could skip this variable

Ah, I wasn't aware of that - I'll get rid of that momentarily.

Commit 9d516c8 fixed this.

lipkau · 2018-05-26T10:09:17Z

JiraPS/Public/Remove-JiraIssue.ps1

+        [Switch]
+        $IncludeSubTasks,
+
+        [PSCredential]


There is a PR (#263) on develop which will change the definition of -Credential.
Please use the changed version, so that I don't have to merge again:

https://github.com/AtlassianPS/JiraPS/pull/263/files#diff-2ebb9ed7b267e9feab2af2ea05fd9422

I think I should be able to rebase onto develop so this is a non-issue.

I've rebased onto develop, which should automatically include #263. AFAIK [PSCredential] should be analogous to [System.Management.Automation.PSCredential], but I also added [System.Management.Automation.CredentialAttribute()] as well so strings are handled appropriately.

lipkau · 2018-05-26T10:50:19Z

docs/en-US/commands/Remove-JiraIssue.md

+Module Name: JiraPS
+online version:
+schema: 2.0.0
+---


Missing some metadata. Eg:

--- external help file: JiraPS-help.xml Module Name: JiraPS online version: https://atlassianps.org/docs/JiraPS/commands/Remove-JiraGroup/ locale: en-US schema: 2.0.0 layout: documentation permalink: /docs/JiraPS/commands/Remove-JiraGroup/ ---

Ah, okay - I wasn't sure if the add'l data was added automatically. I'll get that put in.

Commit 6450878 added the missing metadata.

Only call Get-JiraIssue when needed

hmmwhatsthisdo · 2018-05-29T22:58:08Z

2.) About -Issue
I have some functions stashed which no longer take [Object] as input for -Issue.
This has the consequence of -Issue always to depend on the output of Get-JiraIssue.
But this will happen with the next major version anyway.
If you are interested in making this function only take [JiraPS.Issue] as input, let me know and I will share the code.

@lipkau I'd like to see what you have available in this regard. As an alternative relying on specifically pipeline-based input, we may be able to leverage parameter sets to allow for string-based issue selection (i.e. -Issue vs. -IssueId/IssueKey).

lipkau · 2018-05-31T17:03:58Z

JiraPS/Public/Remove-JiraIssue.ps1

+        [ValidateScript(
+            {
+                if (("JiraPS.Issue" -notin $_.PSObject.TypeNames) -and (($_ -isnot [String]))) {
+                    $errorItem = [System.Management.Automation.ErrorRecord]::new(


This is unfortunatly not compatible with PSv3.
Please update it, as I did with the rest of the module:

https://github.com/AtlassianPS/JiraPS/pull/273/files#diff-762c92178567422918bfa5448fbb1e52R28

Commit 1627090 should have fixed this.

lipkau · 2018-05-31T17:05:05Z

JiraPS/Public/Remove-JiraIssue.ps1

+
+        [System.Management.Automation.CredentialAttribute()]
+        [System.Management.Automation.PSCredential]
+        $Credential,


For compatibility with PSv3 and v4, this requires a default value (because of [System.Management.Automation.CredentialAttribute()])

$Credential = [System.Management.Automation.PSCredential]::Empty

This has been fixed in commit 639c608.

lipkau · 2018-05-31T17:06:28Z

Tests/Remove-JiraIssue.Tests.ps1

+
+        . "$PSScriptRoot/Shared.ps1"
+
+        $script:ShowMockData = $true


Please remove this for the PR once you are done with debugging

Whoops! I completely forgot I'd left that in there. Commit 157dcd4 removed that line.

lipkau · 2018-05-31T17:08:27Z

Tests/Remove-JiraIssue.Tests.ps1

+                "errors": {}
+              }
+"@ | ConvertFrom-Json
+            Resolve-JiraError $MockedResponse -WriteError


You are not supposed to use another function from the module in the UnitTests.
Please use Write-Error instead

Resolved with commit 575b888

lipkau · 2018-05-31T17:10:46Z

Tests/Remove-JiraIssue.Tests.ps1

+                Assert-MockCalled -CommandName Invoke-JiraMethod -Exactly -Times 1 -Scope It
+            }
+
+            It "Throws on issues with subtasks" {


it shouldn't throw.
IMHO it should be a non-terminating error.

as in:

Get-JiraIssue -Query "project= Foo" | Remove-JiraIssue -ErrorVariable $fails

I would expect the command to process all issues... (continuing if one fails)

Right - normally, it would simply write a non-terminating error. However, it seems that Pester can't (directly) check if something wrote an error (at least according to pester/Pester#366) - the only options are to promote it to a terminating error using -ErrorAction Stop or to use -ErrorVariable and check its contents afterwards. I chose the former only because it seemed more concise.

That's fine. but the text of the It block should describe what the function does. In this case, writes an error on issues with subtasks

This was fixed in commit 2e86d9f.

lipkau · 2018-05-31T17:14:15Z

About your comment:

This is how I did it:

JiraPS/JiraPS/Public/Remove-JiraFilter.ps1

Line 6 in 2a53f7c

[PSTypeName('JiraPS.Filter')]

you can see in the rest of the function, the benefits of it.

and yes: I am not opposed to having an additional parameter -Id or similar.
At least for now... I need to do some thinking on this

* Update Remove-JiraIssue to use [PSTypeName()]

lipkau

Excellent job! Thanks

@lipkau

### IMPROVEMENTS - Added support for paginated response from API server by means of `-Paging` (#291, [@lipkau[]]) - Added full set of functions to manage Filter Permissions (#289, [@lipkau[]]) - Added `-Id` parameter to `Remove-JiraFilter` (#288, [@lipkau[]]) - Changed logic of `Get-JiraUser` to return multiple results for a search (#272, [@lipkau[]]) - Added posts for homepage to the module's repository (#268, [@lipkau[]]) - Improved handling of _Credentials_ (#271, [@lipkau[]]) - Added missing interactions with _Filters_ (#266, [@lipkau[]]) - Added `Remove-JiraIssue` (#265, [@hmmwhatsthisdo[]]) - Improved Build script (to deploy changes to the homepage) (#259, [@lipkau[]]) ### BUG FIXES - Reverted `Add-JiraIssueAttachament` as JiraPS v2.7 broke it (#287, [@lipkau[]]) - Fixed resolving of Remote Link (#286, [@lipkau[]]) - Improved error handling for ErrorDetails and non-JSON/HTML responses (#277, [@hmmwhatsthisdo[]]) - Fully support Powershell v3 (#273, [@lipkau[]]) - Fixed parameter used in documentation but not in code (#263, [@lipkau[]])

@lipkau

### IMPROVEMENTS - Added support for paginated response from API server by means of `-Paging` (#291, [@lipkau[]]) - Added full set of functions to manage Filter Permissions (#289, [@lipkau[]]) - Added `-Id` parameter to `Remove-JiraFilter` (#288, [@lipkau[]]) - Changed logic of `Get-JiraUser` to return multiple results for a search (#272, [@lipkau[]]) - Added posts for homepage to the module's repository (#268, [@lipkau[]]) - Improved handling of _Credentials_ (#271, [@lipkau[]]) - Added missing interactions with _Filters_ (#266, [@lipkau[]]) - Added `Remove-JiraIssue` (#265, [@hmmwhatsthisdo[]]) - Improved Build script (to deploy changes to the homepage) (#259, [@lipkau[]]) ### BUG FIXES - Reverted `Add-JiraIssueAttachament` as JiraPS v2.7 broke it (#287, [@lipkau[]]) - Fixed resolving of Remote Link (#286, [@lipkau[]]) - Improved error handling for ErrorDetails and non-JSON/HTML responses (#277, [@hmmwhatsthisdo[]]) - Fully support Powershell v3 (#273, [@lipkau[]]) - Fixed parameter used in documentation but not in code (#263, [@lipkau[]])

hmmwhatsthisdo requested review from a team as code owners May 25, 2018 22:05

ghost added the Status:In Review label May 25, 2018

lipkau assigned hmmwhatsthisdo May 26, 2018

lipkau added Issue:Enhancement Area:Module labels May 26, 2018

lipkau reviewed May 26, 2018

View reviewed changes

lipkau requested changes May 26, 2018

View reviewed changes

lipkau added this to the v2.8 milestone May 26, 2018

lipkau added Status:In Progress and removed Status:In Review labels May 29, 2018

ghost added Status:In Review and removed Status:In Progress labels May 29, 2018

hmmwhatsthisdo added 6 commits May 29, 2018 15:26

Initial implementation of Remove-JiraIssue w/ documentation

0d2bbc1

Make Remove-JiraIssue "Object" param positional;

750cd9f

Only call Get-JiraIssue when needed

Use $issueObj (with implied .ToString()) in $PSCmdlet.ShouldProcess()

2116117

Add additional information to Remove-JiraIssue doc frontmatter

4e71efa

Add alias on -IncludeSubTasks to match Jira API terminology

2b5215a

Forced explicit .ToString on $PSCmdlet.ShouldProcess()

d687cff

hmmwhatsthisdo force-pushed the Remove-JiraIssue_264 branch from 1dc47aa to d687cff Compare May 29, 2018 22:27

Updated -Credential parameter declaration on Remove-JiraIssue

cea2e6d

hmmwhatsthisdo added 2 commits May 30, 2018 14:43

Fix incorrect variable usage in Remove-JiraIssue foreach loop

a8d2165

Add Pester tests for Remove-JiraIssue 🎉

8f1f41e

lipkau requested changes May 31, 2018

View reviewed changes

Merge branch 'develop' into Remove-JiraIssue_264

3bce99e

hmmwhatsthisdo and others added 11 commits May 31, 2018 10:39

Add default value for -Credential

639c608

Remove debugging lines from Remove-JiraIssue tests

157dcd4

Change parameter validation to accommodate PSv3

1627090

Remove unit test dependency on Resolve-JiraError

575b888

Merge branch 'develop' into Remove-JiraIssue_264

ab53595

Merge branch 'develop' into Remove-JiraIssue_264

048ecf8

Add -Cmdlet param to Invoke-JiraMethod inside Remove-JiraIssue

e749422

Fix ambiguous test description (with clarifying comments)

2e86d9f

* Add separate string-based ParamSet to Remove-JiraIssue

9a6825d

* Update Remove-JiraIssue to use [PSTypeName()]

Fix inconsistencies in documentation/sanity-check tests

b3d1ab1

Fix broken tests

167a1ed

lipkau approved these changes Jun 5, 2018

View reviewed changes

lipkau merged commit 538c221 into AtlassianPS:develop Jun 5, 2018

ghost removed the Status:In Review label Jun 5, 2018

hmmwhatsthisdo deleted the Remove-JiraIssue_264 branch June 5, 2018 17:32

lipkau mentioned this pull request Jun 28, 2018

Release of v2.8 #297

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement removal of Jira issues via Remove-JiraIssue #265

Implement removal of Jira issues via Remove-JiraIssue #265

hmmwhatsthisdo commented May 25, 2018 •

edited

Loading

hmmwhatsthisdo commented May 25, 2018 •

edited

Loading

lipkau left a comment •

edited

Loading

lipkau May 26, 2018 •

edited

Loading

hmmwhatsthisdo May 29, 2018

lipkau May 26, 2018

hmmwhatsthisdo May 29, 2018

hmmwhatsthisdo May 29, 2018

lipkau May 26, 2018

hmmwhatsthisdo May 29, 2018

hmmwhatsthisdo May 29, 2018 •

edited

Loading

lipkau May 26, 2018

hmmwhatsthisdo May 29, 2018

hmmwhatsthisdo May 29, 2018

hmmwhatsthisdo commented May 29, 2018 •

edited

Loading

lipkau May 31, 2018

hmmwhatsthisdo May 31, 2018

lipkau May 31, 2018

hmmwhatsthisdo May 31, 2018

lipkau May 31, 2018

hmmwhatsthisdo May 31, 2018

lipkau May 31, 2018

hmmwhatsthisdo May 31, 2018

lipkau May 31, 2018

hmmwhatsthisdo May 31, 2018

lipkau Jun 3, 2018

hmmwhatsthisdo Jun 4, 2018

lipkau commented May 31, 2018

lipkau left a comment

Implement removal of Jira issues via Remove-JiraIssue #265

Implement removal of Jira issues via Remove-JiraIssue #265

Conversation

hmmwhatsthisdo commented May 25, 2018 • edited Loading

Description

Types of changes

Checklist:

hmmwhatsthisdo commented May 25, 2018 • edited Loading

lipkau left a comment • edited Loading

Choose a reason for hiding this comment

lipkau May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmmwhatsthisdo May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmmwhatsthisdo commented May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipkau commented May 31, 2018

lipkau left a comment

Choose a reason for hiding this comment

hmmwhatsthisdo commented May 25, 2018 •

edited

Loading

hmmwhatsthisdo commented May 25, 2018 •

edited

Loading

lipkau left a comment •

edited

Loading

lipkau May 26, 2018 •

edited

Loading

hmmwhatsthisdo May 29, 2018 •

edited

Loading

hmmwhatsthisdo commented May 29, 2018 •

edited

Loading