Closed dragonfist453 closed 1 year ago
Looks good. I was going to recommend to extract to function but the process is a one liner, and the method will be too simple lmao.
Edit: on further thought, it may be useful to extract to a method as a it'll function in case we add more date based conversation
The problem with this is... instead of .toISOString for each var, I will be doing convertToUTC() for each var. Same nonesense. For one line change for each var, it's useless to make a new method
- LGTM, but Instead of using the
toISOString()
method multiple times, can we define a helper function that applies the toISOString() method to the necessary date fields in the object. [Minor]Just read your comment, didn't read it before reviewing. Then it should be fine.
- also please add in a comment why are we doing this conversion so that it can help in future.
I added a helper function for code readability and future reference! It made sense to add all of those lines into one method.
Updated the description to explain the problem and the solution
This is good, 👌🏼 just a smol suggestion since we are using the same concept in 2 files, could we more generic function like this and have it in some
util
files (if we have)function convertDatesInValuestoUTC(obj, keys) { const result = { ...obj }; keys.forEach((key) => { if (result[key]) { result[key] = new Date(result[key]).toISOString(); } }); return result; }
you can call this method like
const requestData = convertDatesInValuestoUTC(values, ['eventstart', 'eventend', 'pubstart', 'pubend']);
This out of scope for this bug, anyway Approving the PR 👍🏼 , Thanks @dragonfist453 :)
Damn. This is why we worship you, sir xD
Shall do! Lovely idea
@Prajwalprakash3722 Proceed with a squash merge if you think the changes now are good!
LGTM, great work @dragonfist453 😃
This is good, 👌🏼 just a smol suggestion since we are using the same concept in 2 files, could we more generic function like this and have it in some
util
files (if we have)function convertDatesInValuestoUTC(obj, keys) { const result = { ...obj }; keys.forEach((key) => { if (result[key]) { result[key] = new Date(result[key]).toISOString(); } }); return result; }
you can call this method like
const requestData = convertDatesInValuestoUTC(values, ['eventstart', 'eventend', 'pubstart', 'pubend']);
This out of scope for this bug, anyway Approving the PR 👍🏼 , Thanks @dragonfist453 :)
Damn. This is why we worship you, sir xD
Shall do! Lovely idea
This is the least I could do
The frontend converts the time object to local time so that the user can read and enter in the date field correctly. On sending this data to the backend, the backend server does a new Date(date) which assumes the time it got is UTC, and simply uses the local time we sent as UTC time. When frontend receives this for the event, we display it as +5:30 of the received time because we got time as UTC.
To avoid this problem, just before submission the values had to be modified to UTC string as the frontend as info about the timezone of the user. Backend does not have that info and should only get UTC, else the logic breaks. This was fixed in this PR, to make sure we only send UTC.