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

Removing sessionToken and authData from _User objects included in a query #1450

Merged
merged 2 commits into from
Apr 12, 2016

Conversation

simonas-notcat
Copy link
Contributor

This bug caused sessionToken to be replaced on client side to some old
sessionToken from DB.

There was a similar issue discussed here: #373 , but the proposed fix had a bug in it.

…uery

This bug caused sessionToken to be replaced on client side to some old
sessionToken from DB.
@flovilmart
Copy link
Contributor

Good catch! Thanks for the PR! Can you add a unit test to prevent subsequent regressions?

@simonas-notcat
Copy link
Contributor Author

Sorry, but I'm not sure how to write a test for that. I was having problems with users, that were migrated from Parse.com.

Anyway, I've already spent to much time on this issue... :(

@flovilmart
Copy link
Contributor

If you had to make a fix, that means there was an error somehow. The unit test will make sure subsequent refactoring don't resurface that error. Without it, that's really problematic for us, as we won't be able to make sure your fix and the behaviour it introduces can be properly maintain over time.

@codecov-io
Copy link

Current coverage is 92.81%

Merging #1450 into master will increase coverage by +0.04% as of eabdb9a

@@            master   #1450   diff @@
======================================
  Files           87      87       
  Stmts         5483    5485     +2
  Branches      1012    1013     +1
  Methods          0       0       
======================================
+ Hit           5087    5091     +4
  Partial         10      10       
+ Missed         386     384     -2

Review entire Coverage Diff as of eabdb9a

Powered by Codecov. Updated on successful CI builds.

@flovilmart
Copy link
Contributor

@simonas-notcat
Copy link
Contributor Author

I don't understand where this line comes from. I can't see it here:
acb294d
simonas-notcat@acb294d

@flovilmart
Copy link
Contributor

it's line 453 of the same file

@simonas-notcat
Copy link
Contributor Author

@flovilmart
Copy link
Contributor

when you opened the link a line what highlighted in yellow, that's a variable that don't seems to be used anywhere.

@flovilmart
Copy link
Contributor

capture d ecran 2016-04-11 a 13 28 37

@simonas-notcat
Copy link
Contributor Author

@flovilmart I can see that line only when I open your link. But when I change Diff view options, it disappears (thats what I tried to show you in my video). Also, I can't see that line in my commit.

@flovilmart
Copy link
Contributor

You didn't changed that line, I'm asking you to remove it as it's unused.

@simonas-notcat
Copy link
Contributor Author

I feel that there is some kind of miscommunication going on. I'm trying to tell you that I did not commit that line. Please open this link and take a look for your self: simonas-notcat@acb294d?diff=unified

@flovilmart
Copy link
Contributor

I know you did not commit that line, I'm asking you to remove it as you removed the reference to className in your commit and we have a dangling variable that is never used.

@facebook-github-bot
Copy link

@simonas-notcat updated the pull request.

@simonas-notcat
Copy link
Contributor Author

@flovilmart I think yesterday I was too tired to figure out what you were asking :). Hope now it's ok

@flovilmart
Copy link
Contributor

Yeah that's great! Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants