Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#32 - Scope Event Index to events that have not happened yet #59

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

exegeteio
Copy link
Contributor

  • Added a scope to the Event model to get ongoing or upcoming Events.
  • Added spec for testing that only future and ongoing events are selected, while past events are not.
  • Updated events controller to select only ongoing or upcoming events.

Copy link
Owner

@ChaelCodes ChaelCodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this, but could you add a feature test? And what happens if it ends today? And our events that only take place over an afternoon, like a meetup.

@exegeteio
Copy link
Contributor Author

I was debating adding this to features/events/index_spec.rb, but got concerned that the feature testing may become brittle if the definition of Event#ongoing_or_upcoming changes.

As for events which end today, those should get included in the existing query.

As for events with only a few hours, I had not considered how those would be effected. I can switch to using Time.zone.now for the comparison, unless it would better serve the user to include those events which happened today?

@ChaelCodes
Copy link
Owner

I was debating adding this to features/events/index_spec.rb, but got concerned that the feature testing may become brittle if the definition of Event#ongoing_or_upcoming changes.

Would you consider a request spec a better choice? Really, we're just verifying the integration there. It only needs one test though. Just verifying that a past event isn't listed.

As for events which end today, those should get included in the existing query.

Thanks!

As for events with only a few hours, I had not considered how those would be effected. I can switch to using Time.zone.now for the comparison, unless it would better serve the user to include those events which happened today?

I think this is good. I can see someone wanting all events that happened today, but that can be an issue when someone wants it.

Added a scope to the Event model to get ongoing or upcoming Events.
Added Factory for past events.
Added spec for testing that only future and ongoing events are selected, while past events are not.
Added spec for ensuring that past events are not presented on the Event index.
Updated events controller to select only ongoing or upcoming events.
@exegeteio
Copy link
Contributor Author

Updated with with a feature spec, comparison to the current time when requesting the Events, and (because it's used in multiple specs) a Factory for :past_event. Hopefully a little humor is welcome in the Factories?

Copy link
Owner

@ChaelCodes ChaelCodes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great!!! This'll make the events page a lot cleaner!✨

@ChaelCodes ChaelCodes merged commit c85223e into ChaelCodes:main Oct 4, 2021
@ChaelCodes ChaelCodes added this to the Hacktoberfest 2021 milestone Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants