search

benedictrobb / beevr

https://beevr.herokuapp.com
8 stars 1 forks source link

Session management #110

Closed antoniotrkdz closed 7 years ago

antoniotrkdz commented 7 years ago

Finally

des-des commented 7 years ago

I have already looked at some of these commits

des-des commented 7 years ago

@majakudlicka @antoniotrkdz I have already looked at some of these commits

des-des commented 7 years ago

@antoniotrkdz

This pull relies on commits that are in another pull I have looked at.

Only assign me to pulls that are ready for review

I have asked you this many times now...

If you have chained your pulls this is the flow.

  1. Wait until previous pulls are merged
  2. Once pulls are in, merge master
  3. assign me

The important thing here is that assigning me is step 3. Do not assign me before a pull is ready for review

antoniotrkdz commented 7 years ago

Sorry Eoin, you are right. I have assigned you the other PR because I belived they were ready for review. Unfortunately, I spotted a bug which I solved in following commits. This is the one that the other PRs are relying on. so maybe you have seen those commits there. If You merge and then I pull in the other branches they will disappear. Sorry for assigning you earlier than due, but I specified the order of review in the comments.

antoniotrkdz commented 7 years ago

I have merged master in. Sorry, but as I said above this is the first one to be merged, so probably you have already seen the changes in those ones.

des-des commented 7 years ago

@antoniotrkdz now you have merged master, the commits that I have already looked at are no longer present.

You can look at the commits here: https://github.com/majakudlicka/beevr/pull/110/commits and the diff here https://github.com/majakudlicka/beevr/pull/110/files

Before you merged, it was showing commits / code that I had already reviewed. It is your job to make sure a pull does not have code already reviewed in the diff before you assign me

antoniotrkdz commented 7 years ago

Also for the last point I now included the relevant info into the cookie at login. So instead of the payload all the relevant endpoint would use request.auth.credentials.... Is this what you asked for? If yes we need to cascade this change on all the affcted routes.

antoniotrkdz commented 7 years ago

I have applied all your comments and I have completed Navbar function after the addition of the burger menu.