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

Allow no main event for a competition #5142

Merged
merged 4 commits into from Feb 6, 2020

Conversation

@jonatanklosko
Copy link
Member

jonatanklosko commented Feb 6, 2020

Follow up for #5140, as it turns out that it is expected for some competitions to have no main event.

@@ -27,6 +27,7 @@ class Competition < ApplicationRecord
has_many :wcif_extensions, as: :extendable, dependent: :delete_all
has_many :bookmarked_competitions, dependent: :delete_all
has_many :bookmarked_users, through: :bookmarked_competitions, source: :user
belongs_to :main_event, class_name: "Event"

This comment has been minimized.

Copy link
@viroulep

viroulep Feb 6, 2020

Member

It's a bit weird to have this be a belongs_to when really it's the competition which has one main event (but yeah given the key is here it's the correct thing).
As far as I understand the code, calling competition.main_event would fire a select right?
Couldn't we just implement the main_event attribute like that:

def main_event
  Event.c_find(main_event_id)
end
@viroulep

This comment has been minimized.

Copy link
Member

viroulep commented Feb 6, 2020

I've went ahead and made some changes to remove the association and leverage the event cache.
I've also change the validation for main_event_id to always run and check inclusion of the value in nil + the competition events (I think this is a requirement at all time).

I'm obviously open to discuss these changes, but I'll merge this asap as it fixes some announced competitions pages; we can adjust internal stuff later :)

@viroulep viroulep force-pushed the jonatanklosko:allow-no-main-event branch from b7a7326 to 4df888c Feb 6, 2020
@viroulep viroulep merged commit ba10359 into thewca:master Feb 6, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 96.087%
Details
@jonatanklosko

This comment has been minimized.

Copy link
Member Author

jonatanklosko commented Feb 6, 2020

Thanks for taking care of this, the changes look good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.