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

Add more info to the competition API #8409

Merged
merged 11 commits into from
Oct 10, 2023

Conversation

FinnIckler
Copy link
Member

To Render the WCA-Registrations Frontend we need some more info as part of the competition API. I also added a stale? check because this route will be used much more heavily on the Frontend and we don't want to recompute it if not needed.

I chose to keep it under it's current route as this change is compatible with the current response, but I can add a new path if it makes more sense.

WcaOnRails/app/models/competition.rb Outdated Show resolved Hide resolved
Comment on lines 20 to 24
COMPETITION_INFO_SERIALIZE_OPTIONS = {
only: %w[id name website start_date registration_open registration_close announced_at cancelled_at end_date competitor_limit extra_registration_requirements enable_donations refund_policy_limit_date event_change_deadline_date waiting_list_deadline_date on_the_spot_registration on_the_spot_entry_fee_lowest_denomination qualification_results event_restrictions base_entry_fee_lowest_denomination currency_code allow_registration_edits allow_registration_self_delete_after_acceptance allow_registration_without_qualification refund_policy_percent use_wca_registration guests_per_registration_limit venue contact force_comment_in_registration use_wca_registration external_registration_page guests_entry_fee_lowest_denomination guest_entry_status information],
methods: %w[url website short_name city venue_address venue_details latitude_degrees longitude_degrees country_iso2 event_ids registration_opened? main_event_id number_of_bookmarks using_stripe_payments? uses_qualification? uses_cutoff? events_with_rounds schedule_wcif],
include: %w[delegates organizers tabs],
}.freeze
Copy link
Member

Choose a reason for hiding this comment

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

This slightly reminds me of thewca/wcif#17.
What information is there in this big array that you need, but that you cannot currently get through WCIF?

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to only have the new fields that I need, but as mentioned above, most of them are not in the wcif

@gregorbg
Copy link
Member

gregorbg commented Oct 5, 2023

I chose to keep it under it's current route as this change is compatible with the current response, but I can add a new path if it makes more sense.

No, as long as it's backwards-compatible a new route is not necessary

@FinnIckler
Copy link
Member Author

only: ["extra_registration_requirements", "enable_donations", "refund_policy_limit_date", "event_change_deadline_date", "waiting_list_deadline_date", "on_the_spot_registration", "on_the_spot_entry_fee_lowest_denomination", "qualification_results", "event_restrictions", "base_entry_fee_lowest_denomination", "currency_code", "allow_registration_edits", "allow_registration_self_delete_after_acceptance", "allow_registration_without_qualification", "refund_policy_percent", "use_wca_registration", "guests_per_registration_limit", "venue", "contact", "force_comment_in_registration", "use_wca_registration", "external_registration_page", "guests_entry_fee_lowest_denomination", "guest_entry_status", "information"]
methods: ["registration_opened?", "main_event_id", "number_of_bookmarks", "using_stripe_payments?", "uses_qualification?", "uses_cutoff?", "events_with_rounds", "schedule_wcif"]
include: ["tabs"]

These are all the fields I added, most of them are not available in wcif

@gregorbg
Copy link
Member

gregorbg commented Oct 6, 2023

These are all the fields I added, most of them are not available in wcif

The schedule_wcif most definitely is available in WCIF 😇
Actually, that one is the only item that I have certain reservations against: It is huge and it is redundant (regarding the fact that it is available in another endpoint).

Is there any chance your new frontend can fetch the WCIF and the "internal competition format" separately and combine the two as needed?

@FinnIckler
Copy link
Member Author

That is no problem, that would also mean I don't need events_with_rounds.

@FinnIckler
Copy link
Member Author

Any idea why the tests fail?
I was expecting json = super(DEFAULT_SERIALIZE_OPTIONS.merge(options || {})) to correctly merge it with the default options and then have fields like id be available.

@FinnIckler
Copy link
Member Author

FinnIckler commented Oct 9, 2023

I just tested it in the ruby console and DEFAULT_SERIALIZE_OPTIONS.merge(COMPETITION_INFO_SERIALIZE_OPTIONS) is just COMPETITION_INFO_SERIALIZE_OPTIONS, so the code in serializable_hash actually never worked (at least for this use case)

@FinnIckler
Copy link
Member Author

looking at the documentation for merge it makes sense:

If no block is specified, the value for entries with duplicate keys will be that of other_hash. Otherwise the value for each duplicate key is determined by calling the block with the key, its value in hsh and its value in other_hash.
So maybe adding a block to the merge function instead will be the day

I added a block to see if that is the way

@gregorbg
Copy link
Member

I solved it using the https://github.com/danielsdeleo/deep_merge third-party library. For context:

  • merge(a, b) simply takes the value from b.
  • deep_merge(a, b) tries to recursively merge, but crucially only works for hashes. Merging arrays inside hashes upon colliding keys is not supported by default :(
    • Providing a block works because it "teaches" Rails how to merge arrays. But it may break when we ever pass nested hashes in the include part.
    • The library dynamically determines what to do. Yay!

@FinnIckler
Copy link
Member Author

and tests pass! We should be able to merge it then, which is going to enable me to change the code on wca-registrations accordingly

WcaOnRails/app/models/competition.rb Outdated Show resolved Hide resolved
@gregorbg gregorbg merged commit 116dcd1 into thewca:master Oct 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants