apostrophecms / apostrophe-events

10 stars 5 forks source link

Set upcoming by time instead of just date #32

Closed bgantick closed 6 years ago

bgantick commented 6 years ago

Issue: an event later today is considered upcoming(false)

close #30

boutell commented 6 years ago

But it's "gte" so why wouldn't it work with just the date?

bgantick commented 6 years ago

@boutell The issue here is that upcoming should account for time and not just date. My wording was goofy.

boutell commented 6 years ago

But $gte would account for that, and also provide a grace period (if it's going on today it's still upcoming). We like that. I think. Sometimes.

On Thu, Sep 20, 2018 at 11:54 AM Brian Gantick notifications@github.com wrote:

@boutell https://github.com/boutell The issue here is that upcoming should account for time and not just date. My wording was goofy.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-events/pull/32#issuecomment-423235348, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB9fTVcwRGDqNDNT9EyMRXbk1bp5v5aks5uc7pMgaJpZM4WyaDJ .

--

THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT P'UNK AVENUE | (215) 755-1330 | punkave.com

bgantick commented 6 years ago

Seems weird to me that an event today would be considered both upcoming and not upcoming. This is an issue when creating past events indexes (an event today shouldn't be considered a past event). Could also just change endDate: { $lte: now } to endDate: { $lt: now }. Interested to hear what others think. I like the idea of taking end time into consideration.

boutell commented 6 years ago

I think for "upcoming: true" you are probably right. It can't be upcoming if we're past its end time. I wouldn't blow it away just for being past its start time though as many events are ongoing and welcome new arrivals etc.

On Thu, Sep 20, 2018 at 12:11 PM Brian Gantick notifications@github.com wrote:

Seems weird to me that an event today would be considered both upcoming and not upcoming. This is an issue when creating past events indexes (an event today shouldn't be considered a past event). Could also just change endDate: { $lte: now } to endDate: { $lt: now }. Interested to hear what others think. I like the idea of taking end time into consideration.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-events/pull/32#issuecomment-423241557, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB9fYebU1ANoo443XTS0t2Ipt9yquUWks5uc74bgaJpZM4WyaDJ .

--

THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT P'UNK AVENUE | (215) 755-1330 | punkave.com

boutell commented 6 years ago

(Principle of least dismay: people should be able to find stuff; seeing slightly too much stuff is a lesser source of dismay)

On Thu, Sep 20, 2018 at 12:43 PM Tom Boutell tom@punkave.com wrote:

I think for "upcoming: true" you are probably right. It can't be upcoming if we're past its end time. I wouldn't blow it away just for being past its start time though as many events are ongoing and welcome new arrivals etc.

On Thu, Sep 20, 2018 at 12:11 PM Brian Gantick notifications@github.com wrote:

Seems weird to me that an event today would be considered both upcoming and not upcoming. This is an issue when creating past events indexes (an event today shouldn't be considered a past event). Could also just change endDate: { $lte: now } to endDate: { $lt: now }. Interested to hear what others think. I like the idea of taking end time into consideration.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apostrophecms/apostrophe-events/pull/32#issuecomment-423241557, or mute the thread https://github.com/notifications/unsubscribe-auth/AAB9fYebU1ANoo443XTS0t2Ipt9yquUWks5uc74bgaJpZM4WyaDJ .

--

THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT P'UNK AVENUE | (215) 755-1330 | punkave.com

--

THOMAS BOUTELL, CHIEF SOFTWARE ARCHITECT P'UNK AVENUE | (215) 755-1330 | punkave.com

bgantick commented 6 years ago

Right, then it goes back to the implementation in this PR: an event will be considered upcoming if the end time is after or equal to now, and not upcoming if the end time is before or equal to now.

plantainrain commented 6 years ago

@bgantick makes sense to me. Would it make more sense if for upcoming the end time is after now, and not after or equal to now? If an event ends at 5pm, and it is 5pm, I would say that event is over.

bgantick commented 6 years ago

Makes sense to me. Updated.

abea commented 6 years ago

Also resolves #30