cicscareers / 320-S20-Track1

Integration Track 1
BSD 3-Clause "New" or "Revised" License
11 stars 3 forks source link

Use moment-timezone library to account for different timezones #399

Closed ishchhabra closed 4 years ago

ishchhabra commented 4 years ago

Implementing the use of UTC timezone throughout the app is done. What is remaining is now converting the timezone back to the user's local timezone when retrieving the data back to the user.

ishchhabra commented 4 years ago

Requires the backend to be reviewed first. https://github.com/cicscareers/320-S20-Track1/issues/393

powellsl99 commented 4 years ago

@ishchhabra Just let me know when the backend gets reviewed and when you want me to review this one

ishchhabra commented 4 years ago

Hey @powellsl99, I just added a temporary method to the API so you could review the frontend before the backend gets reviewed with the updated code. I think you could review the frontend now.

ishchhabra commented 4 years ago

Could some other developers also test out this PR because being such a big edit, there could be areas where I might have left some bugs.

ishchhabra commented 4 years ago

@ishchhabra There appears to be some unexpected behavior with the supporter's names, where sometimes it is the full name, sometimes first only, and sometimes initials only

The backend code was updated to use the preferred name instead of full name. So, is that what you might be referring to? Since I was working on the same file needed for that change, I had made a small change for that as well in the backend.

powellsl99 commented 4 years ago

@ishchhabra There appears to be some unexpected behavior with the supporter's names, where sometimes it is the full name, sometimes first only, and sometimes initials only

The backend code was updated to use the preferred name instead of full name. So, is that what you might be referring to? Since I was working on the same file needed for that change, I had made a small change for that as well in the backend.

Ahhhh okay that makes sense. I think we need to change it a bit then before it should be merged, because I think it is pretty confusing right now. We should make it clear on the frontend if it is supposed to be only first name, or first and last (I think just first is preferable and then we can use their given last name). It is confusing right now, and it wont be helpful to users to only see first names. In addition, we should update the backend so that when a new user signed up, their actual name is set as the default for their preferred name, rather than pn or whatever it is now

ishchhabra commented 4 years ago

@ishchhabra There appears to be some unexpected behavior with the supporter's names, where sometimes it is the full name, sometimes first only, and sometimes initials only

The backend code was updated to use the preferred name instead of full name. So, is that what you might be referring to? Since I was working on the same file needed for that change, I had made a small change for that as well in the backend.

Ahhhh okay that makes sense. I think we need to change it a bit then before it should be merged, because I think it is pretty confusing right now. We should make it clear on the frontend if it is supposed to be only first name, or first and last (I think just first is preferable and then we can use their given last name). It is confusing right now, and it wont be helpful to users to only see first names. In addition, we should update the backend so that when a new user signed up, their actual name is set as the default for their preferred name, rather than pn or whatever it is now

So, basically the frontend doesn’t handle the names and all. Backend already sends the name formatted. What it earlier does was send “first_name last_name”. Now, I changed it to sending preferred name instead if it exists or keep it same if it doesn’t.

I could update the backend for setting a default preferred name on signup.

Is there anything I should do on the frontend to get this PR merged because that wasn’t clear to me completely.

powellsl99 commented 4 years ago

@ishchhabra There appears to be some unexpected behavior with the supporter's names, where sometimes it is the full name, sometimes first only, and sometimes initials only

The backend code was updated to use the preferred name instead of full name. So, is that what you might be referring to? Since I was working on the same file needed for that change, I had made a small change for that as well in the backend.

Ahhhh okay that makes sense. I think we need to change it a bit then before it should be merged, because I think it is pretty confusing right now. We should make it clear on the frontend if it is supposed to be only first name, or first and last (I think just first is preferable and then we can use their given last name). It is confusing right now, and it wont be helpful to users to only see first names. In addition, we should update the backend so that when a new user signed up, their actual name is set as the default for their preferred name, rather than pn or whatever it is now

So, basically the frontend doesn’t handle the names and all. Backend already sends the name formatted. What it earlier does was send “first_name last_name”. Now, I changed it to sending preferred name instead if it exists or keep it same if it doesn’t.

I could update the backend for setting a default preferred name on signup.

Is there anything I should do on the frontend to get this PR merged because that wasn’t clear to me completely.

Wait im confused, where is the name change happening? Because it is only on this PR I see the preferred name. I just think we should fix this before we merge because the UI looks very confusing when all the supporters are just 'pn' or 'name'

ishchhabra commented 4 years ago

@ishchhabra There appears to be some unexpected behavior with the supporter's names, where sometimes it is the full name, sometimes first only, and sometimes initials only

The backend code was updated to use the preferred name instead of full name. So, is that what you might be referring to? Since I was working on the same file needed for that change, I had made a small change for that as well in the backend.

Ahhhh okay that makes sense. I think we need to change it a bit then before it should be merged, because I think it is pretty confusing right now. We should make it clear on the frontend if it is supposed to be only first name, or first and last (I think just first is preferable and then we can use their given last name). It is confusing right now, and it wont be helpful to users to only see first names. In addition, we should update the backend so that when a new user signed up, their actual name is set as the default for their preferred name, rather than pn or whatever it is now

So, basically the frontend doesn’t handle the names and all. Backend already sends the name formatted. What it earlier does was send “first_name last_name”. Now, I changed it to sending preferred name instead if it exists or keep it same if it doesn’t. I could update the backend for setting a default preferred name on signup. Is there anything I should do on the frontend to get this PR merged because that wasn’t clear to me completely.

Wait im confused, where is the name change happening? Because it is only on this PR I see the preferred name. I just think we should fix this before we merge because the UI looks very confusing when all the supporters are just 'pn' or 'name'

So for this PR, there needed to be backend changes as well which I created under a new test lambda function. So, that’s why changes are just seen in this PR because backend changes are not live yet until this PR is approved.

powellsl99 commented 4 years ago

@ishchhabra There appears to be some unexpected behavior with the supporter's names, where sometimes it is the full name, sometimes first only, and sometimes initials only

The backend code was updated to use the preferred name instead of full name. So, is that what you might be referring to? Since I was working on the same file needed for that change, I had made a small change for that as well in the backend.

Ahhhh okay that makes sense. I think we need to change it a bit then before it should be merged, because I think it is pretty confusing right now. We should make it clear on the frontend if it is supposed to be only first name, or first and last (I think just first is preferable and then we can use their given last name). It is confusing right now, and it wont be helpful to users to only see first names. In addition, we should update the backend so that when a new user signed up, their actual name is set as the default for their preferred name, rather than pn or whatever it is now

So, basically the frontend doesn’t handle the names and all. Backend already sends the name formatted. What it earlier does was send “first_name last_name”. Now, I changed it to sending preferred name instead if it exists or keep it same if it doesn’t. I could update the backend for setting a default preferred name on signup. Is there anything I should do on the frontend to get this PR merged because that wasn’t clear to me completely.

Wait im confused, where is the name change happening? Because it is only on this PR I see the preferred name. I just think we should fix this before we merge because the UI looks very confusing when all the supporters are just 'pn' or 'name'

So for this PR, there needed to be backend changes as well which I created under a new test lambda function. So, that’s why changes are just seen in this PR because backend changes are not live yet until this PR is approved.

Okay yeah so then for the backend changes we should make the default preferred name the user's first name, and return "{preferred name} + " " + {lastname}" as 'name'. For this PR, I think we should just update the wording a bit on the profile pages that is is preferred first name, not whole name

powellsl99 commented 4 years ago

@ishchhabra one final small thing before we merge, I think something with times has changed the supporter's scores. By default, all of the supporters now have poor match rather than great match. Could you look into this?

ishchhabra commented 4 years ago

@ishchhabra one final small thing before we merge, I think something with times has changed the supporter's scores. By default, all of the supporters now have poor match rather than great match. Could you look into this?

Hi, I did look into that but was unable to figure out the problem here. So, firstly, that issue isn't something related to this PR but is something related to my other (already approved) PR #383. When we have no search parameters, the matching score is 0, so that's why we get "Poor Match". We could modify the function mapping the scores (mapScore() in supporterCards.js) to have another condition for if s == 0 if that's what you prefer, but that isn't related to this PR and is an issue already existing before this PR.

powellsl99 commented 4 years ago

@ishchhabra one final small thing before we merge, I think something with times has changed the supporter's scores. By default, all of the supporters now have poor match rather than great match. Could you look into this?

Hi, I did look into that but was unable to figure out the problem here. So, firstly, that issue isn't something related to this PR but is something related to my other (already approved) PR #383. When we have no search parameters, the matching score is 0, so that's why we get "Poor Match". We could modify the function mapping the scores (mapScore() in supporterCards.js) to have another condition for if s == 0 if that's what you prefer, but that isn't related to this PR and is an issue already existing before this PR.

Ok. Just please add that in another PR then. Thanks