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] Adding API #60

Merged
merged 16 commits into from
Dec 30, 2022
Merged

[PR] Adding API #60

merged 16 commits into from
Dec 30, 2022

Conversation

LuchoTurtle
Copy link
Member

closes #53

This PR adds the ability for users to interact with Phoenix as an API. It should:

  • create
  • update
  • update status

@LuchoTurtle LuchoTurtle self-assigned this Dec 12, 2022
@LuchoTurtle LuchoTurtle marked this pull request as draft December 12, 2022 20:49
@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Dec 12, 2022

Still very much in-progress but should be manageable.
Was stuck for a while trying to convert an Ecto.Changeset struct to a map but couldn't for a while. Tried several links:

But none were really useful. I really wanted a way of keeping the code clean to simply return a JSON object using Postman. I came across elixir-ecto/ecto#1348 but it was an old issue and didn't really seem the idiomatic way of doing things (even if José gave the answer).

Using Map.from_struct/1 wasn't helpful because it couldn't parse the __meta__ tag inside the changeset.

Came across https://alchemist.camp/episodes/encoding-ecto-structs-poison and it was it (though not at first). The answer was having to add the @derive notation on the top of the schema inside lib/app/todo/item.ex. The person in the video was using Poison but since Jason was being used in the project, I adapted it.

This only works because Jason is being used as the de facto parsing library inside config.exs.

# Use Jason for JSON parsing in Phoenix
config :phoenix, :json_library, Jason

@LuchoTurtle
Copy link
Member Author

The API routes work under the "/api" scope, as per Phoenix 1.7rc's guide and recommendation. (even though we could use content negotiation, I personally don't see a problem with this path and it makes it simpler for the users).

I'm going to update the readme after standup.

@LuchoTurtle
Copy link
Member Author

Finished adding tests. They aren't 100% but they technically are.
image

The missing lines are already covered on #56. Since this PR was created with auth, doesn't make sense to duplicate code and possibly making merging difficult. So it's best to merge #56 first and then this one.

If this PR is merged into main, I can merge main into this branch so coverage gets to 100%.

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #60 (4f61445) into main (acb5d7f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #60   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         9    +1     
  Lines           66        91   +25     
=========================================
+ Hits            66        91   +25     
Impacted Files Coverage Δ
lib/app/todo/item.ex 100.00% <100.00%> (ø)
lib/app_web/controllers/api_controller.ex 100.00% <100.00%> (ø)
lib/app_web/router.ex 100.00% <100.00%> (ø)

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

@LuchoTurtle
Copy link
Member Author

Just added tests, a guide and referred it in the main readme.
As I've stated before in #60 (comment), this PR makes sense to only be merged after #56. The README adds a section numbered 13, taking into account that auth is 12.

@nelsonic
Copy link
Member

@LuchoTurtle please tidy the merge-conflicts when you can. Thanks.

@nelsonic nelsonic assigned LuchoTurtle and unassigned nelsonic Dec 28, 2022
@LuchoTurtle
Copy link
Member Author

@nelsonic should be sorted. 👍

@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Dec 28, 2022
@nelsonic
Copy link
Member

Thanks for fixing the merge-conflicts. 👍

@nelsonic
Copy link
Member

Compiling 22 files (.ex)
warning: router paths should begin with a forward slash, got: "items"
  (phoenix 1.7.0-rc.0) lib/phoenix/router/scope.ex:113: Phoenix.Router.Scope.validate_path/1
  (phoenix 1.7.0-rc.0) lib/phoenix/router/resource.ex:31: Phoenix.Router.Resource.build/3
  lib/app_web/router.ex:35: (module)
  (elixir 1.14.1) src/elixir_compiler.erl:65: :elixir_compiler.dispatch/4
  (elixir 1.14.1) src/elixir_compiler.erl:50: :elixir_compiler.compile/3
  (elixir 1.14.1) src/elixir_module.erl:379: :elixir_module.eval_form/6

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.

Great additions; thanks @LuchoTurtle! 🎉

@nelsonic nelsonic merged commit 6a9657d into main Dec 30, 2022
@nelsonic nelsonic deleted the api branch December 30, 2022 01:53
HotategaiDev added a commit to HotategaiDev/phoenix-todo-list-tutorial- that referenced this pull request Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Feat: Todo List REST API
2 participants