gcivil-nyu-org / team-5-inperson

3 stars 4 forks source link

Token session password val upd username adnan #194

Closed AdnanAhsanNYCity closed 1 year ago

AdnanAhsanNYCity commented 1 year ago

Travis CI needs to be fixed. Draft for now.

guptaviha commented 1 year ago

What features does this PR include? Would be helpful if you could list them out so I know what to test in my review

AdnanAhsanNYCity commented 1 year ago

What features does this PR include? Would be helpful if you could list them out so I know what to test in my review

Yeah no worries! Here is the list below:

AdnanAhsanNYCity commented 1 year ago

Hi,

Thanks for the details! Here is my feedback:

  1. I noticed that there now seems to be a delay in loading detail of an amenity with many reviews - maybe due to the extra logic in fetching username? Is this something you noticed as well? Not a showstopper though, probably something we should look to optimize later.
  2. I found a bug with password validation. FE says it should be between 8 and 25 chars. But the BE has a max length of 8.
Screenshot 2022-11-28 at 3 29 35 PM

I think its a simple fix (check UserSerializer in serializers.py). Sanity checks can help catch any bugs like this early on.

  1. Question: Will commenting the BE-force-logout logic mess up the flow of the token timeout feature? Im guessing if the FE and BE arent in sync, it may introduce unexpected bugs? Should we hold off on pushing the feature? What do you think?
  2. I noticed you added --ignore=F401 to travis. If we comment the import lines, we wouldn't have to ignore the linter
Screenshot 2022-11-28 at 3 38 48 PM
  1. Thanks for adding amenity ID to the detail page. A simple and super useful feature :)

Hi,

Fixed #2 and #4.

For #1 I believe it's because of how we got the username through backend. We take the userid from the detail page, compare it to the user table and get the username, and then we propagate the username to the detail page. I believe we can optimize this later but for now this is what we were able to get up and running.

For #3 the FE and BE are supposed to be synced but independent of each other to ensure they don't cause conflict/bugs. So it should be okay. Im trying to get BE up right now, we might have to use Django settings to get the BE working. Something along the lines of using "Browser Expire at Close", a setting Django has which has something else called "Middleware" which I think can modify the setting so that the session expires later than at close. I'm researching that now, and will try to implement it by tonight.

I'll be working on getting #3 fixed but even without that the PR should be ok. Lmk what y'all think. (I will likely make a separate PR for that backend fix)

5 Tysm!

Best, Adnan

AdnanAhsanNYCity commented 1 year ago

Update on the backend logout: It'll be much more complicated than expected. So this will be done in a different PR, this current iteration of this PR is the final one.