worldcubeassociation.org icon indicating copy to clipboard operation
worldcubeassociation.org copied to clipboard

Added results/personal_records/:user_id API endpoint

Open dunkOnIT opened this issue 2 years ago • 5 comments

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_records route - not sure if this is of concern
  • Question 1: I'm currently getting ranksSingle and ranksAverage and then transforming the data in the controller - should I instead have an indexedRanksSingle method 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": {
        ....
    }
}}

dunkOnIT avatar Dec 29 '23 08:12 dunkOnIT

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:

  1. Store the date_achieved (or event just date_created?) on ranksSingle and ranksAverage. This would be hard to backfill, as there's no link from the ranks tables to the attempt date/competition date.
  2. Store latest_date_achieved instead, 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)
  3. 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

  1. 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.
  2. 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.

dunkOnIT avatar Jan 03 '24 11:01 dunkOnIT

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.

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.

gregorbg avatar Jan 03 '24 16:01 gregorbg

Ok great thanks! Will continue with the current behaviour then.

dunkOnIT avatar Jan 04 '24 10:01 dunkOnIT

(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!)

dunkOnIT avatar May 09 '24 16:05 dunkOnIT

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.

dunkOnIT avatar May 31 '24 07:05 dunkOnIT

This PR is cleaned up + ready for review

dunkOnIT avatar Jul 04 '24 08:07 dunkOnIT

@thewca-bot deploy staging

dunkOnIT avatar Jul 08 '24 08:07 dunkOnIT