Skip to content

Conversation

dkodippily
Copy link
Contributor

Added new test class AbstractAuthenticationTargetUrlRequestHandlerTests to confirm AbstractAuthenticationTargetUrlRequestHandler. determineTargetUrl() behaviour.

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @dkodippily! I've left some feedback inline. Also, once you get to refactoring the class under test, will you please squash your commits like so:

  1. one commit for the tests, followed by
  2. one commit for the refactor

The first commit should end with Issue gh-12344 and the second comment should end with Closes gh-12344.

@jzheaux jzheaux changed the title Gh 12344 Rewrite AbstractAuthenticationTargetUrlRequestHandler#determineTargetUrl logic for clarity Jan 5, 2023
@jzheaux jzheaux self-assigned this Jan 5, 2023
@jzheaux jzheaux added in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 5, 2023
@jzheaux jzheaux added this to the 6.1.0-M1 milestone Jan 5, 2023
@dkodippily
Copy link
Contributor Author

Thanks, @dkodippily! I've left some feedback inline. Also, once you get to refactoring the class under test, will you please squash your commits like so:

  1. one commit for the tests, followed by
  2. one commit for the refactor

The first commit should end with Issue gh-12344 and the second comment should end with Closes gh-12344.

@jzheaux Thanks for the comments , I've added the changes

Copy link
Contributor

@jzheaux jzheaux left a comment

Choose a reason for hiding this comment

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

Thanks, @dkodippily! I've left some feedback inline about the latest changes.

@dkodippily
Copy link
Contributor Author

Thanks, @dkodippily! I've left some feedback inline about the latest changes.

Thanks , @jzheaux , I've pushed the changes.

@marcusdacoregio marcusdacoregio modified the milestones: 6.1.0-M1, 6.1.0-M2 Jan 16, 2023
@jzheaux jzheaux merged commit 879770a into spring-projects:main Jan 18, 2023
@jzheaux
Copy link
Contributor

jzheaux commented Jan 18, 2023

Thanks, @dkodippily! This is now merged into main. I also added a polish commit to simplify the if statements down to unary conditions.

@dkodippily
Copy link
Contributor Author

Thanks, @dkodippily! This is now merged into main. I also added a polish commit to simplify the if statements down to unary conditions.

Thank you @jzheaux for the support and guidance, planning to pick another issue soon :)

@dkodippily dkodippily deleted the gh-12344 branch February 24, 2023 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants