fix: enhance get_user_if_exists pipeline function with debugging capabilities

[Akanshu-2u]

Description:

The get_user_if_exists function lacks visibility into authentication failures. We can't debug why "user already exists" errors happen in devstack but not in stage.

Solution:

• Added comprehensive debugging with new toggle IGNORE_LOGGED_IN_USER_ON_MISMATCH.

• Added custom attributes to track authentication flow and detect issues:

      - Username mismatches between details and existing users.
      - User lookup success/failure states.
      - Database errors during user retrieval.
  

• Improved logging with detailed info and warning messages.

• Removed unnecessary pass statement for cleaner code.

JIRA Ticket:

BOMS-3

Reference PR:

PR

Reference pr for testing legacy get_user_if_exists code with current test_file:

#395

[robrap]

I did not review tests yet. I'll review once the code has been updated. Thank you.

[robrap]

Thanks.

[robrap]

As written, I was having difficulty digesting the different tests and ensuring complete coverage and that each test was doing what we wanted to do. I know you are just exploring ddt for the first time, so no worries. I'm providing some feedback that may be useful to everyone who might use ddt at some point.

[robrap]

I know there are lots of comments here, but it was soooo much easier to digest and understand the tests. Thank you for all the changes. We're almost there.

[robrap]

@Akanshu-2u: I haven't gotten to the review yet. However, can you please add me as a collaborator with write access on your private fork? Thank you!

[Akanshu-2u]

@Akanshu-2u: I haven't gotten to the review yet. However, can you please add me as a collaborator with write access on your private fork? Thank you!

Added you as a colaborator for my fork.

[robrap]

@Akanshu-2u: I haven't gotten to the review yet. However, can you please add me as a collaborator with write access on your private fork? Thank you!

Added you as a colaborator for my fork.

@Akanshu-2u: Strange. You added me having write access? I ask because I still do not have permissions to resolve PR comments when I am done reviewing.

[robrap]

• I don't see an internal reviewer approval since my last comments, and I was told it was ready for review.
• Many of the comments say things were addressed, but I don't see any of the changes or new commits. Again, if an internal reviewer were to have looked at my comments and looked at github, this should have been caught before it gets to me.
• Lastly, lots of the comments say "Addressed", but some of my comments have no response. When you do commit changes, please ensure that you respond to all of my comments, and that you actually addressed them.
  Thank you.
  FYI: @Akanshu-2u @vgulati-apphelix

[jcapphelix]

I've added a few comments,

if you do go with my suggestions/proposals, you'll need to change few places to get the user like

existing_user = self.user

or

different_user = User.objects.create(**self.details['different_user_details'])

All in all if those changes are implemented, it'll be a bit of extra work on this, but it will improve reading the code and reduce repeating of user creation in each test case.

All those are nits, and are just my thoughts on this.

@robrap @Akanshu-2u

[jcapphelix]

Looks good from my side.

[robrap]

Looks great. One small optional nit. Thank you. Just let me know if you intend to change or skip.

Also, one additional request to ensure we didn't break any of the legacy implementation. Could you copy the new tests into the current code, and make sure they pass. You can delete out all the ddt changes, because there will be no toggle and we just want to test legacy behavior. You will also need to delete test conditions that won't be entered, but leave the other test condition. Delete new observability assertions, etc. The point is to ensure that the newer tests (or at least a simplified version of them) pass against the old code, to ensure that we didn't add a regression into the new code and capture it in a test that is pretending all is well.

Let me know if this needs more clarity. I think all is fine, but I just want to be certain. Thank you.

[robrap]

@Akanshu-2u: I added some additional changelog and testing requests. Also see #390 (review) for some testing requests and an optional minor update.

[Akanshu-2u]

Looks great. One small optional nit. Thank you. Just let me know if you intend to change or skip.

Also, one additional request to ensure we didn't break any of the legacy implementation. Could you copy the new tests into the current code, and make sure they pass. You can delete out all the ddt changes, because there will be no toggle and we just want to test legacy behavior. You will also need to delete test conditions that won't be entered, but leave the other test condition. Delete new observability assertions, etc. The point is to ensure that the newer tests (or at least a simplified version of them) pass against the old code, to ensure that we didn't add a regression into the new code and capture it in a test that is pretending all is well.

Let me know if this needs more clarity. I think all is fine, but I just want to be certain. Thank you.

Yes, I addressed this suggestion and to be very certain about the tests replicated these with the legacy code. After the tests passed with the legacy code , I went onto create a PR with the modified test file which focus just on testing and not for merging.

[robrap]

Minor comment . Thank you. No need for internal review.

[robrap]

@Akanshu-2u: After you address the one comment, please mark all comments resolved, including the latest ones. Thank you.

[Akanshu-2u]

@idegtiarov The pr is reviewed and approved by @robrap . If you are able to approve, squash and release the library, that would be great. Thank you!

We plan to remove temporary toggles once we've proven this safe

UPDATE: Please see PR description for details.

[robrap]

@Akanshu-2u: You can edit your comment to remove the duplication of the PR description. It is not needed, because it is all in the description. You could instead have written: "Please see PR description for details.". Thank you.

[robrap]

Providing an updated approval as well.

[robrap]

[inform] This PR didn't quite accomplish what we had wished, once we were better able to test. This issue is being closed, and we're working on a different proposal that may logout the current user before starting the login process.