fix: Send event startsAt and endsAt time in event timezone #3633

prateekj117 · 2019-11-16T19:22:28Z

Fixes #3632

Short description of what this resolves:

Send event timings in event timezone and render time correctly on event basic details page.

Checklist

I have read the Contribution & Best practices Guide.
My branch is up-to-date with the Upstream development branch.
The acceptance, integration, unit tests and linter pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

prateekj117 · 2019-11-16T19:34:44Z

@iamareebjamal Please review.

iamareebjamal · 2019-11-16T20:03:54Z

app/utils/computed-helpers.js

@@ -47,13 +47,13 @@ export const computedSegmentedLink = function(property) {
 export const computedDateTimeSplit = function(property, segmentFormat, endProperty) {
  return computed(property, {
    get() {
-      return moment(this.get(property)).format(getFormat(segmentFormat));
+      return moment(this.get(property)).tz(this.timezone).format(getFormat(segmentFormat));


Will fail for everything except event

@prateekj117 Dont send the timezone with time, as we are already collecting timezone in seperate field. I dont think sending timezone with date & time would be mandatory.

It is mandatory. As I said, please read about moment and timezones and ISO 8601 format a bit more

@iamareebjamal So adding a if conditional should work in that case here.

iamareebjamal · 2019-11-18T13:06:45Z

@prateekj117 Status?

prateekj117 · 2019-11-18T17:59:40Z

@iamareebjamal I am having an issue in case of startsAt and endsAt time in speakers and sessions. The page crashes when I try this.event.timezone in case of session and speakers.

iamareebjamal · 2019-11-18T18:53:36Z

Just handle the case for event wizard right now and open an issue to handle others

kushthedude · 2019-11-18T18:55:10Z

Use a if condition just to handle the event case, session and speaker can be handled after the release.

…

On Tue, 19 Nov, 2019, 00:23 Areeb Jamal, ***@***.***> wrote: Just handle the case for event wizard right now and open an issue to handle others — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3633?email_source=notifications&email_token=AKQMTLSFEJ42CV5FNDVDYVTQULQDFA5CNFSM4JOGQOQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEELQMXA#issuecomment-555157084>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLUFMGYWYIBHZ4IS76LQULQDFANCNFSM4JOGQOQA> .

prateekj117 · 2019-11-19T18:14:26Z

@iamareebjamal Please review.

app/utils/computed-helpers.js

prateekj117 · 2019-11-19T21:34:03Z

@iamareebjamal Please review.

iamareebjamal · 2019-11-19T21:35:16Z

app/utils/computed-helpers.js

      } else {
-        return moment(this.get(property)).format(getFormat(segmentFormat));
+        timezone = this.event.get('timezone');


This is worse than before

Idea is to write less code, not more

So, make this work in least code possible, please

I don't want to assign a variable outside if as it may crash the page of the other model inside an if condition.
For example:

let timezone = this.timezone; if(this.constuctor.modelName !== 'event') { timezone = this.event.get('timezone'); }

This may break in session-speakers page, as it will first search for this.timezone.

iamareebjamal · 2019-11-19T21:40:46Z

Normally, I'd wait for you to figure out this so you learn and develop an insight of how to write concise code and not duplicate anything, but since this PR has already delayed the release a lot, this is what you should do:

    let momentDate = moment(this.get(property));
    if (this.constructor.modelName === 'event')
        momentDate = momentDate.tz(this.timezone)
    return momentDate.format(getFormat(segmentFormat));

iamareebjamal · 2019-11-19T21:42:48Z

And as said previously, only handle event case as we don't know the effects of changing behaviours of sessions and tracks. And we don't have devs and time to test it out. They are not immediate bugs and fixing them may break the site even more. As discussed, they are planned for next release. Perfection is the killer of pragmatism

iamareebjamal · 2019-11-19T21:44:20Z

app/utils/computed-helpers.js

+        } else {
+          timezone = this.event.get('timezone');
+        }
+        oldDate = moment(this.get(property), segmentFormat === 'date' ? FORM_DATE_FORMAT : FORM_TIME_FORMAT).tz(timezone, true);


Somehow you managed to increase the duplication even more from previous iteration

iamareebjamal · 2019-11-19T21:48:57Z

app/utils/computed-helpers.js

+        timezone = '';
+        if (this.constructor.modelName === 'event') {
+          timezone = this.timezone;
+        } else {
+          timezone = this.event.get('timezone');
+        }


I don't even understand why this block of code is repeated here in this if statement.

niranjan94 · 2019-11-19T21:55:37Z

Here is an overview of what got changed by this pull request:

Complexity increasing per file
==============================
- app/utils/computed-helpers.js  2

See the complete overview on Codacy

prateekj117 · 2019-11-19T21:56:38Z

@iamareebjamal Please review.

kushthedude · 2019-11-20T09:23:31Z

@prateekj117 Are you sure the time is being sent into event timezone after this ?

kushthedude · 2019-11-20T09:23:59Z

I tried it and time was stored in my browser timezone only .

iamareebjamal · 2019-11-20T09:29:27Z

Yes, I tested it personally

iamareebjamal · 2019-11-20T09:29:54Z

time was stored in my browser timezone only

How did you test that?

kushthedude · 2019-11-20T09:40:05Z

I pulled the changes and created new event and accessed the database to see the time.

…

On Wed, 20 Nov, 2019, 14:59 Areeb Jamal, ***@***.***> wrote: time was stored in my browser timezone only How did you test that? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3633?email_source=notifications&email_token=AKQMTLR5ET5WWDMM4EKU46TQUT7RLA5CNFSM4JOGQOQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEERKMSA#issuecomment-555918920>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLQBOAIXSCM3VUU327DQUT7RLANCNFSM4JOGQOQA> .

iamareebjamal · 2019-11-20T09:43:31Z

And what did you find? Please tell completely. What you did, which timezone you selected, what time did you input and what did you find in DB, and what you expected to find in the DB instead.

Steps of Reproduction, Expected, Actual.

kushthedude · 2019-11-20T09:58:38Z

Its working fine, I checked the time for older event. Sorry for the confusion.

…

On Wed, 20 Nov, 2019, 15:13 Areeb Jamal, ***@***.***> wrote: And what did you find? Please tell completely. What you did, which timezone you selected, what time did you input and what did you find in DB, and what you expected to find in the DB instead. Steps of Reproduction, Expected, Actual. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3633?email_source=notifications&email_token=AKQMTLSDRCV4JPT46MVYJSTQUUBELA5CNFSM4JOGQOQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEERLWRY#issuecomment-555924295>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLW24X7K5MUJB7FX7TDQUUBELANCNFSM4JOGQOQA> .

fix: Send event startsAt and endsAt time in event timezone

fbb6011

auto-label bot added the fix label Nov 16, 2019

iamareebjamal reviewed Nov 16, 2019

View reviewed changes

Merge branch 'development' into event-timezone

40e9eb1

Handle cases for other models

9c93fa3

iamareebjamal reviewed Nov 19, 2019

View reviewed changes

app/utils/computed-helpers.js Outdated Show resolved Hide resolved

iamareebjamal reviewed Nov 19, 2019

View reviewed changes

app/utils/computed-helpers.js Outdated Show resolved Hide resolved

iamareebjamal reviewed Nov 19, 2019

View reviewed changes

Remove duplication

d126fc2

prateekj117 force-pushed the event-timezone branch from 34103b4 to d126fc2 Compare November 19, 2019 21:53

iamareebjamal approved these changes Nov 19, 2019

View reviewed changes

iamareebjamal merged commit f3fbb16 into fossasia:development Nov 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Send event startsAt and endsAt time in event timezone #3633

fix: Send event startsAt and endsAt time in event timezone #3633

prateekj117 commented Nov 16, 2019

prateekj117 commented Nov 16, 2019

iamareebjamal Nov 16, 2019

kushthedude Nov 17, 2019

iamareebjamal Nov 17, 2019

kushthedude Nov 18, 2019

iamareebjamal Nov 18, 2019

iamareebjamal commented Nov 18, 2019

prateekj117 commented Nov 18, 2019

iamareebjamal commented Nov 18, 2019

kushthedude commented Nov 18, 2019 via email

prateekj117 commented Nov 19, 2019

prateekj117 commented Nov 19, 2019

iamareebjamal Nov 19, 2019

iamareebjamal Nov 19, 2019

iamareebjamal Nov 19, 2019

prateekj117 Nov 19, 2019

iamareebjamal commented Nov 19, 2019

iamareebjamal commented Nov 19, 2019

iamareebjamal Nov 19, 2019

iamareebjamal Nov 19, 2019

niranjan94 commented Nov 19, 2019

prateekj117 commented Nov 19, 2019

kushthedude commented Nov 20, 2019

kushthedude commented Nov 20, 2019

iamareebjamal commented Nov 20, 2019

iamareebjamal commented Nov 20, 2019

kushthedude commented Nov 20, 2019 via email

iamareebjamal commented Nov 20, 2019

kushthedude commented Nov 20, 2019 via email

fix: Send event startsAt and endsAt time in event timezone #3633

fix: Send event startsAt and endsAt time in event timezone #3633

Conversation

prateekj117 commented Nov 16, 2019

Short description of what this resolves:

Checklist

prateekj117 commented Nov 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal commented Nov 18, 2019

prateekj117 commented Nov 18, 2019

iamareebjamal commented Nov 18, 2019

kushthedude commented Nov 18, 2019 via email

prateekj117 commented Nov 19, 2019

prateekj117 commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal commented Nov 19, 2019

iamareebjamal commented Nov 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niranjan94 commented Nov 19, 2019

prateekj117 commented Nov 19, 2019

kushthedude commented Nov 20, 2019

kushthedude commented Nov 20, 2019

iamareebjamal commented Nov 20, 2019

iamareebjamal commented Nov 20, 2019

kushthedude commented Nov 20, 2019 via email

iamareebjamal commented Nov 20, 2019

kushthedude commented Nov 20, 2019 via email