veritus / veritus-backend

1 stars 0 forks source link

Remove nested duplicates #60

Closed Ragnar-H closed 6 years ago

Ragnar-H commented 6 years ago

Description

Due to some hacky solutions by yours truly, we've got some duplicates in the data returned by various HTTP calls

f.x. GET http://localhost:8000/api/v1/politicians/

Note that there are more endpoints and serializers that I hacked. These should also be fixed

[
  {
    "name": "Andrés Ingi Jónsson",
    "id": 1,
    "initials": "AIJ",
    "districtNumber": 10,
    "party": {
      "name": "Vinstri Hreyfingin - Grænt Framboð",
      "id": 3,
      "website": "http://vg.is/",
      "created": "2017-04-25T17:41:28Z",
      "modified": "2017-04-25T17:41:28Z"
    },
    "district": {
      "name": "Reykjavik Norður",
      "id": 1,
      "abbreviation": "Reykv. N.",
      "politicians": [
        {
          "name": "Andrés Ingi Jónsson",
          "id": 1,
          "initials": "AIJ",
          "party": {
            "name": "Vinstri Hreyfingin - Grænt Framboð",
            "id": 3,
            "website": "http://vg.is/",
            "created": "2017-04-25T17:41:28Z",
            "modified": "2017-04-25T17:41:28Z"
          },
          "promises": []
        },
        {
          "name": "Birgir Ármannsson",
          "id": 7,
          "initials": "BÁ",
          "party": {
            "name": "Sjálfstæðisflokkur",
            "id": 1,
            "website": "http://xd.is/",
            "created": "2017-04-25T17:41:28Z",
            "modified": "2017-04-25T17:41:28Z"
          },
          "promises": []
        },
...

I hacked this together when we just wanted to get going with some UI components.

Steps to reproduce

  1. Navigate to localhost:8000
  2. Use Swaggers "Try Me" on GET politicians

Expected result

Since we're Reduxing the frontend, we want to normalize state e.g. use ids and expose first-level endpoints

[
  {
    "name": "Andrés Ingi Jónsson",
    "id": 1,
    "initials": "AIJ",
    "districtNumber": 10,
    "party": {
      "id": 3,
    },
    "district": {
      "id": 1,
      "politicians": [
                  1, 7 
        ]
...

Note that districtNumber is NOT the same as districtId

Ragnar-H commented 6 years ago

@skabbi this might be a decent medium sized issue to deal with if you want

skabbi commented 6 years ago

@Ragnar-H I'm a bit unclear on the expected results. Should there be a "politicians" field under "district"?

AriHrannar commented 6 years ago

Yes

Instead of it being

"district": {
   "politicians": [
   { 
      "name": "bbla",
      "id": 1,
   },
   {
      "name": "yeah",
      "id": 2
   }
   ]
}

It should be

"district": {
   "politicians": [ 1, 2 ]
}

As in, it should not return the Politician object, but the ids of each Politician

I imagine it using something like PrimaryKeyRelatedField for serialization

This can also be done in some other serializers. Not 100% sure which, but that requires some quick investigation :)

skabbi commented 6 years ago

Quick question. The "district" field seems to be the same as when calling localhost:8000/api/v1/districts/, e.g.

[
    {
        "name": "Reykjavik Norður",
        "id": 1,
        "abbreviation": "Reykv. N.",
        "politicians": [
            {
                "name": "Andrés Ingi Jónsson",
                "id": 1,
                "initials": "AIJ",
                "party": {
                    "name": "Vinstri Hreyfingin - Grænt Framboð",
                    "id": 3,
                    "website": "http://vg.is/",
                    "created": "2017-04-25T17:41:28Z",
                    "modified": "2017-04-25T17:41:28Z"
                },
                "promises": []
            },
            ....
        ],
        "created": "2017-04-25T17:41:28Z",
        "modified": "2017-04-25T17:41:28Z"
    },
   ...
]

Should the districts endpoint remain unchanged?

skabbi commented 6 years ago

Some additional questions.

  1. What is a districtNumber?
  2. Should the district object only contain the "id" and "politicians" fields or should it continue to contain "name", "abbreviation", "created" and/or "modified".
  3. How would the politician IDs be used?
  4. Would it make sense to instead of returning a duplicate district object for each politician in the same district, to return a districtId instead?
AriHrannar commented 6 years ago
  1. If i remember correctly the Icelandic translation is "Kjördæmanúmer" (see here) which I think is which seat they got from their district. Each district has X amount of seats (lets say 10 for district Y) so someone is nr 1, someone is nr 2 etc

  2. All objects that inherit the Entity object contain those fields. I think its nice to have created / modified for all objects just to keep track of them. Name is also pretty widely used for most objects. Abbreviation is sometimes used but nice to have the option.

  3. On the frontend it will be possible to "getPoliticianById", where you supply the id and get the json object.

  4. Yes, return the districtId :)

skabbi commented 6 years ago

@AriHrannar Thanks for the reply, but I still have a few questions/comments.

  1. I see, good explanation. A minor point; google translate says "Kjördæmi" translates to constituency not district, but I don't think it's worth changing if we all know what if refers to.

  2. Ok, just wanted to make sure since the examples given for expected results don't include them.

  3. So is the idea that when viewing a politician in the UI, it calls "getPoliticianById" for each ID and show a list of all politicians in the same district? My main reason for the original question was that this seems like data the UI should be getting from the districts endpoint, but I might be wrong.

  4. I'm not sure we are on the same page. So just to be sure; I'm talking about since the main point of this issue is to remove duplicate data, that instead of returning e.g. "district": { "id": 1, "politicians": [1, 7, ...], ... }, for each politician in same district, to simply return "districtId: 1" so data about districts can be fetched as needed.

Lastly, it would be nice to get an answer to my original question. "Should the districts endpoint remain unchanged?" Since the politician endpoint is sharing code with the districts endpoint.

AriHrannar commented 6 years ago
  1. Thats probably more correct :)
  2. :+1:
  3. Either we fetch all politicians and store them in the frontend state and then use getPoliticianById there OR we use the /politicians service and call it with a district query parameter to filter politicians by district. Im not sure what is better but I imaging calling the /politicians service with a district query parameter involves less code. @Ragnar-H thoughts?
  4. Yes thats correct

Lastly: Would be nice to also change the districts endpoint to return a list of politician ids instead of a list of politicians :)

AriHrannar commented 6 years ago

Im gonna hijack this :)