-
Notifications
You must be signed in to change notification settings - Fork 17
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
Ensure profileId is set when testing API token #77
Ensure profileId is set when testing API token #77
Conversation
…ntains no log lines with the profileId)
@Owaan Thank you for this PR! Can you explain a bit more the reasoning behind the changes? Is the current code for associating profiles with tarkov tracker tokens not working as intended? |
@Razzmatazzz All good! I've fixed two issues in this PR caused by the Issue 1:Fixed by this commit Testing Steps:
Expected Result: Actual Result: Solution: return Task.Run<string>(() => {
return tokens[currentProfile];
}); I'm now passing the profileId into the Issue 2:Fixed by this commit Testing Steps:
Expected Result: Actual Result: Solution: |
Thanks for the explanation, and for the fixes! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm misunderstanding things, I think some of these changes can be streamlined a bit. Thanks again for the PR!
@Razzmatazzz oh yeah you might be right I think I misread something and also forgot to use the profileId I passed in anyways lol... Will clean it up in a bit, test and recommit |
@Razzmatazzz All changes fixed, first time using C#, apologies that wasn't so clean the first go around I've just used your existing The main issue was in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks again! Can you update the branch so it isn't out of date from the main branch? That's a result of my merging your other PR.
Resolves #73
Not sure if the profile ID needs to be mapped to the API key as TarkovTracker isn't separating profiles (yet). This should do the job though 👍
This also fixes the scenario where a user could be confused launching TarkovMonitor before launching Tarkov or having a profile reported in their logs. You'll still need to handle launching TarkovMonitor without even having Tarkov installed though....