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

[PR] API Stage 2 - Adding tags #280

Merged
merged 35 commits into from
Jan 28, 2023
Merged

[PR] API Stage 2 - Adding tags #280

merged 35 commits into from
Jan 28, 2023

Conversation

LuchoTurtle
Copy link
Member

related to #256

This PR adds tag support, alongside some refactoring related with router helpers.

@LuchoTurtle LuchoTurtle added enhancement New feature or enhancement of existing functionality in-progress An issue or pull request that is being worked on by the assigned person elixir Pull requests that update Elixir code labels Jan 23, 2023
@LuchoTurtle LuchoTurtle self-assigned this Jan 23, 2023
@LuchoTurtle LuchoTurtle marked this pull request as draft January 23, 2023 14:39
@codecov
Copy link

codecov bot commented Jan 23, 2023

Codecov Report

Merging #280 (e295ced) into main (420aef5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main      #280   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        15    +1     
  Lines          414       475   +61     
=========================================
+ Hits           414       475   +61     
Impacted Files Coverage Δ
lib/app_web/router.ex 100.00% <ø> (ø)
lib/api/item.ex 100.00% <100.00%> (ø)
lib/api/tag.ex 100.00% <100.00%> (ø)
lib/app/item.ex 100.00% <100.00%> (ø)
lib/app/tag.ex 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@LuchoTurtle LuchoTurtle mentioned this pull request Jan 23, 2023
@LuchoTurtle
Copy link
Member Author

CRUD tags should now be properly working.
The API tests have also been updated with this new introduction, as well as seeds.exs.

@LuchoTurtle
Copy link
Member Author

Update on this.

I've been implementing the tag array to return proper errors. I've tried different approaches to this. I'm not sure whether I should tell the user that an item already exists or just ignore a tag that exists and add others that do not, and return a 200 successful request.

I've implemented validators for both checking if a tag already exists and if any tag is invalid. However, the current implementation ignores tags that already exist.

Additionally, I'm doing separate DB transactions for each tag instead of inserting all in the same so the insert_at and updated_at fields are correctly setup. I'm aware I could pass it on manually but I think this makes the code more readable and simpler, as I don't have to manually add these fields this way.

@LuchoTurtle
Copy link
Member Author

Trying to implement the embedding of tags when retrieving items but I can't seem to idiomatically implement proper json encoding.
I'm toggling tag loading depending on the embed query parameter passed on by the person using the API but I keep getting errors from Jason trying to encode it.

I've changed the Jason.Encoder annotation in the item schema to the following:

  @derive {Jason.Encoder, except: [:__meta__, :__struct__, :timer, :inserted_at, :updated_at]}

But I keep getting this annoying error:

no function clause matching in Jason.Encoder.App.Item."-inlined-encode/2-"/2

even after deleting the tag key from the map.

I've tried the approaches of these three links but none helped :(

@LuchoTurtle
Copy link
Member Author

I can't seem to get this to work, after countless attempts. I asked the question in Elixir Forums, hopefully I have an answer:

https://elixirforum.com/t/error-json-encoding-struct-no-function-clause-matching-in-jason-encoder-app-item-inlined-encode-2-2/53423

@nelsonic
Copy link
Member

@LuchoTurtle thank you for giving a good account of your progress in this PR and opening the question on the Forum. 👍
Let's discuss in-person tomorrow morning. 🙏

@LuchoTurtle
Copy link
Member Author

Fetching items with tags is now possible. Tested and documented. Added new test suites to the CI, as well.

Going to check docs for any typos and then submit for review.

@LuchoTurtle LuchoTurtle requested a review from nelsonic January 26, 2023 17:44
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jan 26, 2023
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Jan 26, 2023
@nelsonic
Copy link
Member

@LuchoTurtle PR is still "Draft":

image

Are you done with your editing? 💭

@LuchoTurtle LuchoTurtle marked this pull request as ready for review January 26, 2023 18:24
@nelsonic nelsonic added in-review Issue or pull request that is currently being reviewed by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Jan 27, 2023
api.md Outdated Show resolved Hide resolved
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Thanks @LuchoTurtle 👍

@nelsonic nelsonic merged commit 01c40ab into main Jan 28, 2023
@nelsonic nelsonic deleted the api_tags-#256 branch January 28, 2023 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
elixir Pull requests that update Elixir code enhancement New feature or enhancement of existing functionality in-review Issue or pull request that is currently being reviewed by the assigned person
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants