Skip to content

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.

4 participants