fix: Parse.ParseUser.LogOutAsync optimization

[YBTopaz8]

Fixed an issue Parse LogoutAsync's test would not pass.

Closes #400

Now All user tests so far pass hopefully better coverage too.;

[parse-github-assistant]

Thanks for opening this pull request!

• ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Closes: #123 in the PR description, so I can recognize it.

[YBTopaz8]

Curious to know the coverage now

[YBTopaz8]

@mtrezza to help you out,
I did ZERO new addition to the code. I simply forked multiple times and fixed the specific bug/scope.
If you need an order through which you should merge, consider this as the First One to approve.

[mtrezza]

Could you just take a look at the warnings in the CI regarding unused vars, etc? There may be some clean-up needed.

[mtrezza]

I wonder why this PR contains so many changes while it is supposed to fix only 1 failing test. For example, what do the changes in Parse.Tests/AnalyticsTests.cs have to do with the user logout issue?

[YBTopaz8]

Hi @mtrezza .

The reason for those is because I needed to cleanup tests which

1. You requested I do
2. I mentioned in my last commit message
3. It's likely the first change we see when reviewing changes.
   
   All of these were cleanups and re-adjustments (Except Object Tests which only has even more test I forgot to strip out).
   like changing from async void to async task otherwise the test might not run at all or as expected.

    [TestMethod]
    ~public async void TestGetCurrentInstallation()~ THIS
    public async Task TestGetCurrentInstallation() - is now this
    {

the one file with changes that were not warranted was ObjectTest and that's because As I had done my overall fix over 2days ago, I had to ensure tests work and I even added more tests for coverage.
After being asked to separate, that must've been the only file I forgot to reamend after copying and pasting.

If you wish for me to remove all the code cleanups, I will, but that will lead to warnings and errors that I would frankly not circle back to as these will be redundant.

If you wish for me to remove the additional tests codes from ObjectTest so as to stick to the scope... sure. I'll do that.
Just tell so and I'll delete the additional lines and push.

As of now, I am happy and they all pass and any specific change is either a cleanup or a code TEST addition.
Should You wish for me to change anything, I will. Should an issue arise after a change , I'll investigate and if the reason is due to the change I made on your requests, I'll inform you at least and leave them as such.

I hope this fully clears things up for all now.
Thank you.

[mtrezza]

I understand that splitting a large PR into smaller ones, like here, can be challenging as the changes are often entangled. Which is why a PR should focus on a single issue and any optimization that is not part of the touched code should move into other PRs. But let's merge this, so we can get ahead with the other PRs.

[mtrezza]

Was there an actual bug in the code or was this merely a test issue?

[YBTopaz8]

Only tests
It's the Tests test were an issue since I wasn't good at those one - until now

[mtrezza]

Could you explain the changes in

• Parse/Platform/Users/ParseUser.cs
• Parse/Infrastructure/Execution/UniversalWebClient.cs

They look like more than simple refactors.

[YBTopaz8]

• ParseUser.cs
  Changed get => ContainsKey("authData") ? AuthData["authData"] as IDictionary<string, IDictionary<string, object>> : null;
  to

get
        {
            if (ContainsKey("authData"))
            {
                return this["authData"] as IDictionary<string, IDictionary<string, object>>;
            }
            else
                return null;
        }

It's literally the same thing, just expanded them. for readibility.

• Parse/Infrastructure/Execution/UniversalWebClient.cs

Stream data = httpRequest.Data;
        if (data != null || httpRequest.Method.Equals("POST", StringComparison.OrdinalIgnoreCase))
{
            message.Content = new StreamContent(data ?? new MemoryStream(new byte[0]));
}

to ensure that Data is Always passed in http request as sometimes it would be null

if ((httpRequest.Data is null && httpRequest.Method.ToLower().Equals("post")
             ? new MemoryStream(new byte[0])
             : httpRequest.Data) is Stream { } data)
{
            message.Content = new StreamContent(data);

}

Other changes are just done due to copy pasting since I had to split them

[YBTopaz8]

Explained here.

[mtrezza]

to ensure that Data is Always passed in http request as sometimes it would be null

That sounds like a separate issue, more than a refactor. I thought the additional changes were just refactors. I'm having doubts now; how much work would it be to strip this PR down to the bare issue of Parse User login?

[YBTopaz8]

Reverted those changed.
I do not believe we need to open a PR for variable assignment placement.

[mtrezza]

The refactors are not the issue, I said earlier that we can just merge them. By "refactor" I mean simple code restructuring like the revert in 9b4e168 that does not change the logic.

But you mentioned earlier that the changes in UniversalWebClient.cs ...

ensure that Data is Always passed in http request as sometimes it would be null

How is that related to the logout issue? Is that a bug or did I misunderstand your comment?

I also see some code being removed like timeouts:

using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); // Timeout after 30 seconds
...
Timeout = TimeSpan.FromSeconds(15) // Ensure timeout is respected.

How are these changes related to the logout issue? I also didn't see these timeouts moved to another place, they were just removed. That to me looks like more than a "refactor" but actually a change in logic. Or have I missed something in the changes page?

Sorry for being so persistent; I want to get this PR merged quickly, but these things are still unclear to me.

[YBTopaz8]

Sure.

• Let me start by giving you the context through which I was in when doing the PR.
  I when I did PR one that was merged, It was a core system update for it to be using new approaches rather than legacy here and there.

• That over all update brought issues linked to my lack of proper testing skills at the time but all the tests remaining at that time (189 instead of 200 initially) passed so 11 tests were missing (- out of my notice) but we both realized some tests didn't pass, notably Logout and Test Link.

• The core issue for logout rested on the UniversalWebClient.cs file because the NEW approach, at the time, wasn't accounting for when the logout URL doesn't exist anymore (Like Facebook auth) as well as moment when Parse gave an error.

And that needed some time because the server replies back, so I added a slightly higher timeout - NOT necessary but useful to give enough time for a round trip even if (and especially) when rejected.
Thing is, That was code was removed afterwards. because they were MY workaround.

In #404, I removed it since the code structure was now much more understandable to me, so I didn't need to rely much on this timeout approach.
Hence why I changed some various assignment positions because when testing I was to ensure DataTransfer goes first. (Parse Relations didn't get transferred before)

Like I said as we had v4.0.0 merged, I immediately went back to at least increase test coverage or fix immediate bugs since I had feedback on my end too, given that my update had broken Parse LQ.

So It felt all the more natural to push a full corrective update with all fixes AND more coverages, which was my aim ultimately - given that I had just learned and understood, I wanted to give back.

But these separation led to more questions that answer.

1. All of your questions stem from the fact that I copy/pasted my solutions from the main branch where I fixed to a 2nd branch just for logout.
2. All of your question where I answer "it is just a placement change" are pretty much just.. that. Nothing deeper and if that doesn't suit., sure I'll revert those changes.

My aim with the first PR was to quickly push a more stable and overall reliable code. Hence why I was curious about the code coverage in the beginning. But with all these tests failing online and passing on my end, then back and forths I'm just hoping it fixes the issues people face using the official update.
(I pushed my updated 3 days ago and so far so good)

[YBTopaz8]

I'll hereby explain to you WHAT each change mean. I preface by saying too ; this is not a test on your knowledge or so.
I understand we are busy and would prefer to have things clear all at once.
So, I'll explain;

• ParseUser: 106 & 114: removed debug lines, only good for Parse SDK dev
  175: spaced out the line into multiple for readability

• UWC : I simplified the code as the previous one was a workaround to do the logout. If you look through, I had an if-else on 82 that did the same http actions but logout was done slightly differently since at the time I needed more to be considered, so I had splitted first (as seen in v4.0.0). then recombined after fixing, as seen in this PR

[mtrezza]

I had an if-else on 82 that did the same http actions but logout was done slightly differently since at the time I needed more to be considered

Makes more sense to me, I think we can go ahead now. Thanks for taking the time to lay it all out. We'll merge this as a fix, not a mere test refactor, because of the changes in the UWC.

[mtrezza]

Looks good

[parseplatformorg]

🎉 This change has been released in version 4.0.1