Properly return sprites as an object when querying it using GraphQL

[simonorono]

Needed to accomplish this:

• Change the data type of the sprites column in pokemon_v2_pokemonsprites table to JSONB. That makes Hasura to automatically return it as an object.
• Fix the building of the pokemon_v2_pokemonsprites table. It was inserting a string in the field making it be non-valid JSON causing Hasura to return it as a string.
• Fix serializer function get_pokemon_sprites to return the sprites field in the PokemonSprite object directly.

Closes #614

[simonorono]

The build test is failing due to Sqlite not supporting the JSONB type.

[C-Garza]

The build test is failing due to Sqlite not supporting the JSONB type.

Yeah I ran into that when trying to play around with the change without the docker containers at first. Idk how we want to handle this, as postgres does support it, which is what the docker containers use. Do we want to support two different setups?

[simonorono]

The build test is failing due to Sqlite not supporting the JSONB type.

Yeah I ran into that when trying to play around with the change without the docker containers at first. Idk how we want to handle this, as postgres does support it, which is what the docker containers use. Do we want to support two different setups?

@Naramsim do you have a suggestion?

[Naramsim]

Hi, I haven't had the time to test the pr locally. For this week I'm off 😭

Do we really need to change the schema of the DB in such a way that SQLite stops working?

[Naramsim]

Tbh I'd like to continue to support SQLite, since for local testing is pretty handy.

Maybe there's a plugin for enabling jsonb?

[Naramsim]

https://sqlite.org/draft/jsonb.html this? Idk

[C-Garza]

https://sqlite.org/draft/jsonb.html this? Idk

That's crazy, that talks about introducing support for JSONB in version 3.44.0, which just came out on the first of this month and is the newest version! 😮

[simonorono]

I think our problem is the Django version (currently 2.2).

Django 3.1 introduced support for JSON field: https://docs.djangoproject.com/en/3.1/ref/models/fields/#jsonfield

https://sqlite.org/draft/jsonb.html this? Idk

That's crazy, that talks about introducing support for JSONB in version 3.44.0, which just came out on the first of this month and is the newest version! 😮

I don't think Django 2.2 would support that 😅

[Naramsim]

Otherwise I could just use this branch for the graphql server.

Or maybe we could put everything conditionally! If the db is postgresql, run the migration and insert JSONs directly. If it's SQLite we don't run the migration and we insert strings.

I'd go with the second one if you have some more time to spend on the code 😁😁😁😁

[C-Garza]

Yeah if we want to support two types, then we could probably use .env variables for like, a DEV and or PROD environments perhaps? Because sqlite is only used for testing locally in this, right? Or is it used in other places besides locally? 🤔

If we do split the branches, then we'd have to maintain two different branches when one gets updated.

[simonorono]

@Naramsim @C-Garza are we not OK with upgrading the Django version? Because I upgraded it to 3.1 and got the tests running again in SQLite. One test broke and needs fixing:

ERROR: test_pokemon_api (pokemon_v2.tests.APITests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/code/pokemon_v2/tests.py", line 5029, in test_pokemon_api
    response.data["sprites"]["front_default"],
TypeError: string indices must be integers

[C-Garza]

Hmm, if everything works fine still I don't see any harm updating it. But I am not that familiar with all the django packages being used.

[Naramsim]

@simonorono

If you can you're most welcome to upgrade Django :) I tried sometime ago but had some difficulties, I don't remember which ones though. :(

[simonorono]

@Naramsim @C-Garza

Tests are now passing and we are on Django 3.1. Please don't merge it yet, it seems I broke the Docker image during the upgrade but I'm not sure if it's an actual error or a me error.

Also, I've noticed we don't use all packages listed in requirements.txt and now it's a good opportunity to clean that up. I'll get to that during the weekend.

[Naramsim]

😮 That's so nice! Thanks a lot for the hard job!!!

[Naramsim]

Hi! I pushed this code to the staging branch. Let's see the tests and the build there.

[simonorono]

@Naramsim the deploy step is trying to pull the pokeapi/pokeapi image with tag master:

master: Pulling from pokeapi/pokeapi
Digest: sha256:400accc6a2a9ef7ed313050a59c46f7fc834c9af82e1e28f980b4cf6a8f9744b
Status: Downloaded newer image for pokeapi/pokeapi:master

Which has not been generated in the last 12 days (at the moment of writing). I think this is why my Docker environment is also broken.

[Naramsim]

Can you try with the docker compose dev?

docker-compose -f docker-compose.yml -f docker-compose-dev.yml up -d

[simonorono]

@Naramsim with the command you provided the image did build locally and there was no error. Already performed the cleanup of the requirements.txt file. This ticket is ready to be merged.

[Naramsim]

@simonorono , if you still have time could you do the same job for the item sprites and the pokemon forms?

🫣😁

[simonorono]

@Naramsim I forgot about those. Done.

[simonorono]

Ok I was wrong and it was not done. Now it is 😅

[Naramsim]

Hey @simonorono , would you like to join the pokeapi org ?

[simonorono]

Hey @simonorono , would you like to join the pokeapi org ?

Yes :) thank you.