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

fixes 320 - assignment when similar users exist #328

Merged
merged 9 commits into from
Jan 11, 2019

Conversation

michalporeba
Copy link

@michalporeba michalporeba commented Jan 9, 2019

Description

Adds -Exact parameter to the Get-JiraUser which internally switches from using search to get user.
It is then used in the Set-JiraIssue when the -Assignee is set.

Motivation and Context

closes #320
closes #49
is continued by #306
Assigning users failing when Get-JiraUser for a specific username returns more than one user

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.

@michalporeba michalporeba requested review from a team as code owners January 9, 2019 08:36
@ghost ghost added the Status:In Review label Jan 9, 2019
@michalporeba
Copy link
Author

I have a unit test to cover the -Exact option and modified one to be more explicit how the Get-JiraUser should work when the -Exact is not used. I have also updated the Get-JiraUser documentations.

@stevenyoungs
Copy link

Hi @michalporeba
I believe there are many places where Get-JiraUser is called, internally, and each would need to be investigated and potentially suffixed with the new -Exact switch. Additionally, any external call to Get-JiraUser would need to be evaluated and potentially suffixed with -Exact.
The approach I took with #306 was to modify the default behaviour of Get-JiraUser so that it does what your -Exact switch does. The upside as I see it is that, for the most common case of finding an exact user, no further source modifications are required; it 'just works!' As mentioned elsewhere, it is a breaking change and @lipkau, understandably, wants to wait for the next major release.
Much as I would personally love to see this problem fixed sooner, I don't disagree with the decision to await the next major release.

@lipkau do you have a schedule in mind for releasing 3.0?

@michalporeba
Copy link
Author

As far as I see the failing tests are to do with documentation, I would need some guidence there to make it work, but let's talk about the two solutions.

Your change @stevenyoungs fixes much more than what I'm trying to achieve. The filter appears to be good long term solution, but it is a breaking change. Not only internally, but in any script people might have written using it.

My approach has only two things in mind. Fix the problem with assigning users and make sure the impact is minimal. The -Exact switch has the default value of $false and when it is false, there are no changes to the behaviour of JiraPS. The behaviour changes only when the switch is set to $true, and that is used only in one place in Set-JiraIssue. That makes the change very localised and easy to test. While it doesn't resolve much bigger problem, it solves one specific bug and could be successfully used in the current bersion before 3.0 is ready. Also, because it is a fix to that specific issue and not a replacement for your solution to the bigger problem I don't see why the -Exact would have to be added to the other commands.

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.

regarding the test failures:

JiraPS/Public/Get-JiraUser.ps1 Outdated Show resolved Hide resolved
Tests/Functions/Get-JiraUser.Unit.Tests.ps1 Outdated Show resolved Hide resolved
Tests/Functions/Get-JiraUser.Unit.Tests.ps1 Show resolved Hide resolved
@lipkau
Copy link
Member

lipkau commented Jan 9, 2019

@stevenyoungs : I don't think this PR is the ideal solution for this matter.
but as it has been raised a lot of times (and is a very old issue), I would allow this change to pass and refactor it with #306 for the next major version. hope you agree

@michalporeba
Copy link
Author

Thanks for the information and review. I will make the changes tomorrow morning.

lipkau
lipkau previously approved these changes Jan 10, 2019
@lipkau lipkau changed the base branch from master to develop January 10, 2019 16:20
@michalporeba
Copy link
Author

Thank you @lipkau for solving the documentation problem for me. Can you explain what it was? Was it just the missing ### -Exact above the yml section I have added? Is the parameter type difference important? I have seen in other places SwitchParameter used and you changed it to Switch.

@lipkau
Copy link
Member

lipkau commented Jan 11, 2019

### is markdown for <h3/>.
So the name of the parameter must be the header for the description and yaml table.

you can see how that looks like in html on the documentation page:
https://atlassianps.org/docs/JiraPS/commands/Get-JiraUser/#parameters

@lipkau lipkau merged commit 18e876d into AtlassianPS:develop Jan 11, 2019
@ghost ghost removed the Status:In Review label Jan 11, 2019
@lipkau lipkau added this to the Next Minor milestone Jan 11, 2019
@lipkau lipkau mentioned this pull request Feb 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants