Added results/personal_records/:user_id API endpoint
Intention: Expose personal records information which the Registration Service backend can use to verify qualifications.
Questions/Notes:
- Putting it under
/results, as this should ultimately be provided by a Results Service - Shares a lot of similarities with Finn's
/users/me/personal_recordsroute - not sure if this is of concern -
Question 1: I'm currently getting
ranksSingleandranksAverageand then transforming the data in the controller - should I instead have anindexedRanksSinglemethod on the model? (I don't want to introduce a method on the model that is only used in one place and returns similar data to another method - seems better to use the existing method and transform the data. ) - Question 2: Just noticed the methods use camelCase not snake_case - should we change that?
- Question 3: Should we group PR's under single/average -> event_id, or event_id -> single/average? Do we care? (see below for example)
- Question 4: Does it matter that I'm removing fields that Finn exposes in /users/me/personal_records? (specifically: personId, id)
WCIF Personal Bests are defined as an array of these JSON objects:
[{
"eventId": "222",
"best": 311,
"worldRanking": 2894,
"continentalRanking": 726,
"nationalRanking": 22,
"type": "single"
}]
The /user/me/personal_records PR iterates on this by grouping them as average or single, and adding some new fields (in bold:
{ "single": [
{
**"id": 1451696265,**
"**personId": "2019HOBB02",**
"eventId": "222",
"best": 513,
"worldRank": 52323,
"continentRank": 442,
"countryRank": 260
},
]}
This PR returns data in the following format, adding eventId as a key on the personal_record object, and removing id and personId
{ "single": {
"222": {
"best": 513,
"worldRank": 52323,
"continentRank": 442,
"countryRank": 260
},
}}
To me, it would be more intuitive to group by event -> single/average, as follows:
{ "222": {
"single": {
"best": 513,
"worldRank": 52323,
"continentRank": 442,
"countryRank": 260
},
"average": {
....
}
}}
Ok this gets WAY more complicated when trying to include the date on which a result was achieved, because ranksSingle and ranksAverage (where we are currently getting the PR's from) don't store them.
I see a few options for code changes, in order of my preference:
- Store the
date_achieved(or event just date_created?) onranksSingleandranksAverage. This would be hard to backfill, as there's no link from therankstables to the attempt date/competition date. - Store
latest_date_achievedinstead, and default all values to the date on which the field is added. Very easy to populate, and should work unless there are qualifications for competitions with registrations still open that have a date requirement before the date of the change (which I think is unlikely, but haven't verified) - Explore changing the data model to have PersonalBest somehow linked to the User and the Result - this could definitely work, but would represent a significant migration, perhaps more appropriate for the results rework?
Alternatives
- Alternatively, we move the qualification logic entirely - and have a
GET /competition/{comp-id}/check_qualification/{user_id}endpoint that uses. I really don't like this, because it generates an uncacheable REST call for every single registration, which means that traffic traffic on the monolith scales with registration count, which sort of defeats what we're trying to do. - Add this as a requirement for the Results rework, and don't support qualifications with date requirements on the new Registration System until the Results rework is done.
Ok this gets WAY more complicated when trying to include the date on which a result was achieved, because
ranksSingleandranksAverage(where we are currently getting the PR's from) don't store them.
For some high-level context: This fact, which you correctly identified, is one of our biggest pain points in the current Results tables from a software point of view. It not only affects qualifications but also rankings and regional records (NRs/CRs/WRs). It definitely has to be tackled as part of the Results redesign.
As such, I suggest simply skipping this feature in the new Reg service "for now" (the Monolith doesn't have automated qualification checks for a given deadline, either. It can only check whether qualification requirements are fulfilled at the time of filling out the registration form, which should be feasible in the new Reg service as well) and implementing it later when we have the proper data available.
Ok great thanks! Will continue with the current behaviour then.
(Busy adding tests for this which I should have done in the first place, just had to fix some DB issues - hope to get it sorted soon!)
Tests added, ready for review again
**NOTE: ** Worth double-checking the merge commit - it seemed to me like the api#records route was duplicated on main, so I removed the duplicate entry and left the POST regstration-data in place.
This PR is cleaned up + ready for review
@thewca-bot deploy staging