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 Location and RegistrationInfo objects to root #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JonEsparaz
Copy link
Contributor

Closes #16.

Open to suggestions on the spec.

For RegistrationInfo, I decided to just include a subset of the fields stored in the website's Competition table that relate to registration. For example, I excluded on_the_spot_registration and on_the_spot_entry_fee_lowest_denomination since it wouldn't serve a purpose for Speedcubing-Canada/speedcubing-canada-web#48. However, if we feel that a more exhaustive object would be better, I am happy to write the spec and include that in the PR to update the API. My opinion is that we should add fields if/when they become useful.

For Location, as mentioned in the object's spec, there could be some overlap with Venue. However, I think it's good to include both in the export since the data is not guaranteed to be identical. The spec mentions 3x3x3 Fewest Moves simultaneous competitions, but there is also the possibility that, for example, Location points to the main entrance to a university campus and the one Venue points to a specific building.

@JonEsparaz
Copy link
Contributor Author

@gregorbg (or other WST members), any thoughts on this spec? Thanks!

Copy link
Member

@gregorbg gregorbg left a comment

Choose a reason for hiding this comment

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

Including more information about the registration is fine. I am however strongly opposed to exposing what you called Location information on the top level of the WCIF.

The information that you enter in the competition form (and that gets displayed on the competition frontpage, for that matter) comes from a time when competitions were small and local. It has technically been completely obsolete ever since introducing the concept of "Venues" to a competition (in the codebase),

If anything, I'd rather beef up the Venue model, add information like address and city over there, and give the overall competition an option to mark venues as a "main venue". This implies changes to the current competition form, which means we need to wait at least until thewca/worldcubeassociation.org#7459 (aiming for this month)

specification.md Outdated Show resolved Hide resolved
@@ -556,6 +593,32 @@ Represents the competition data related to time and scheduling.
}
```

### Location
Copy link
Member

Choose a reason for hiding this comment

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

I definitely have to veto this part of the changeset, as it is largely redundant with the "venues" part.

Copy link
Member

Choose a reason for hiding this comment

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

More information/thoughts in my general review comment.

@JonEsparaz
Copy link
Contributor Author

Appreciate the great feedback as always, @gregorbg!

I'll come back to this when thewca/worldcubeassociation.org#7459 is merged in.


```json
{
"registrationOpen": "2023-08-29T05:00:00Z",
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the idea of doing .registrationInfo.registrationOpen. That names are redundant. We could have just .registration.openDate or for consistency, .registration.startDate

Copy link
Contributor

@timreyn timreyn left a comment

Choose a reason for hiding this comment

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

Would you be open to pushing ahead with everything except the Location / Venue changes before thewca/worldcubeassociation.org#7459 is merged in? Let me know if I can help!

@@ -196,6 +211,28 @@ Represents person registration data.
}
```

### RegistrationInfo
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you be open to including competitorLimit here as well? I'd like to use this, along with registrationOpen and registrationClose, for Cube New England's website, so I can say "87 / 125 spots filled".

@JonEsparaz
Copy link
Contributor Author

Would you be open to pushing ahead with everything except the Location / Venue changes before thewca/worldcubeassociation.org#7459 is merged in? Let me know if I can help!

@timreyn yes, that sounds like a good plan/compromise. I can incorporate your feedback and Cailyn's feedback in the next week or so.

@timreyn
Copy link
Contributor

timreyn commented Dec 11, 2023

Hi @JonEsparaz just checking in on this? Let me know if I can help, I should have some free time over the holidays.

@JonEsparaz
Copy link
Contributor Author

@timreyn apologies - this fell off my radar. I likely won't have time to implement this myself soon. If you want to give it a go, please feel free.

timreyn added a commit to timreyn/wcif that referenced this pull request Dec 16, 2023
@timreyn
Copy link
Contributor

timreyn commented Dec 16, 2023

@timreyn apologies - this fell off my radar. I likely won't have time to implement this myself soon. If you want to give it a go, please feel free.

Sure, I opened #23 which is mostly copied from this PR. Thanks!

gregorbg pushed a commit that referenced this pull request Mar 8, 2024
* Add RegistrationInfo to the competition.

Part of #16; stolen from @JonEsparaz's #17.

* Include RegistrationInfo in the example.

* Walk-in -> on-the-spot.

* Delete duplicate competitorLimit.

* Specify that registration fees are in the lowest denomination.

* Fix typo.

* Fix typo.

* usesWcaRegistration -> useWcaRegistration

for consistency with the database.

* Fix example.

* Change lowestDenomination to iso.

* Rename field to baseEntryFee.
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.

Expose city name and registration information (open date/time, base fee)
4 participants