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] Stage 1 - No auth, anonymous API for Timers and Items #263

Merged
merged 27 commits into from
Jan 16, 2023
Merged

Conversation

LuchoTurtle
Copy link
Member

related to #256

This PR creates a first, basic, fully-tested version of a RESTful API for manipulating data anonymously with timers and items.
Also creates a document to detail its development.

@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 labels Jan 12, 2023
@LuchoTurtle LuchoTurtle self-assigned this Jan 12, 2023
@codecov
Copy link

codecov bot commented Jan 12, 2023

Codecov Report

Merging #263 (dceaaa0) into main (158bdde) will decrease coverage by 0.26%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##              main     #263      +/-   ##
===========================================
- Coverage   100.00%   99.73%   -0.27%     
===========================================
  Files           12       14       +2     
  Lines          312      378      +66     
===========================================
+ Hits           312      377      +65     
- Misses           0        1       +1     
Impacted Files Coverage Δ
lib/api/item.ex 100.00% <100.00%> (ø)
lib/api/timer.ex 100.00% <100.00%> (ø)
lib/app/item.ex 100.00% <100.00%> (ø)
lib/app/timer.ex 100.00% <100.00%> (ø)
lib/app_web/router.ex 100.00% <100.00%> (ø)
lib/app_web/live/app_live.ex 99.11% <0.00%> (-0.89%) ⬇️

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

@LuchoTurtle
Copy link
Member Author

Following @SimonLab's advice in #263 (comment), I previously had a try/rescue construct to catch Ecto.CastErrors if the id wasn't an integer, and return the error to the person.

Since I couldn't really use guards to check if id of item/timer was an integer, I just manually checked it and added conditions.

However, I feel using JSON schema validators is the right way to go. I didn't use them yet because using them just to check for an id is super overkill.
I was planning on using Goal but didn't because of this.

Should I have?

@LuchoTurtle LuchoTurtle changed the title [PR] Stage 1 - No auth anonymous API for Timers and Items [PR] Stage 1 - No auth, anonymous API for Timers and Items Jan 12, 2023
@nelsonic
Copy link
Member

https://github.com/dwyl/mvp/actions/runs/3906526566/jobs/6674798916#step:8:127
image

  1) test timers update_timer(%{id: id, start: start, stop: stop}) with start later than stop 
should throw error (App.TimerTest)
Error:      test/app/timer_test.exs:100
     ** (KeyError) key :id not found in: 
%{model: %App.Item{__meta__: #Ecto.Schema.Metadata<:loaded, "items">, 
id: 4, person_id: 1, status: nil, text: "some text", 
timer: #Ecto.Association.NotLoaded<association :timer is not loaded>, 
tags: #Ecto.Association.NotLoaded<association :tags is not loaded>, 
inserted_at: ~N[2023-01-12 22:24:06], updated_at: ~N[2023-01-12 22:24:06]}, 
version: %PaperTrail.Version{__meta__: #Ecto.Schema.Metadata<:loaded, "versions">, id: 4, 
event: "insert", item_type: "Item", item_id: 4, 
item_changes: %{id: 4, inserted_at: ~N[2023-01-12 22:24:06], person_id: 1, 
status: nil, text: "some text", updated_at: ~N[2023-01-12 22:24:06]}, 
originator_id: 1, origin: nil, meta: nil, inserted_at: 
~U[2023-01-12 22:24:06Z]}}
Warning:      code: Timer.start(%{item_id: item.id, person_id: 1, start: seven_seconds_ago})
     stacktrace:
       test/app/timer_test.exs:111: (test)

@nelsonic
Copy link
Member

@LuchoTurtle tests failing due to me merging main branch into this one after merging your PaperTrail PR #255 💭
Feel free to revert changes if you think it will fix the tests. Or we can pair on fixing the tests tomorrow morning. 🧑‍💻

@nelsonic
Copy link
Member

@LuchoTurtle https://github.com/martinthenth/goal looks decent. 👍
Please consider opening a separate issue for it: https://github.com/dwyl/technology-stack/issues 🆕
I'm more inclined to keep this as dependency-free as possible for now. 🔰

@LuchoTurtle
Copy link
Member Author

This should be done.
The coverage "dropped" because of the line we all know it's covered but excoveralls skips, for some reason 😭

@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jan 13, 2023
@nelsonic
Copy link
Member

@LuchoTurtle you've assigned to me but still in-progress? 👨‍💻
Please LMK if you feel it's awaiting-review. 👌

@nelsonic
Copy link
Member

@LuchoTurtle please open a new issue for the bug you spotted. 🐛 🆕 🙏 (thanks!)

@LuchoTurtle
Copy link
Member Author

The bug is in the implementation in this PR, I just need to make a couple of changes to it. No reason to merge this until I get it fixed 😅

@LuchoTurtle
Copy link
Member Author

Found it after assigning it for review, my fault on that. But it's if quick resolution, just assign it back to me when it's convenient for you

@nelsonic
Copy link
Member

Cool. I've done a review and made a few minor updates. 🧑‍💻
Assigning back to you now for the update. 👌
You shouldn't have any more issues with CodeCov complaining about 1 line not being covered. 🤞
Assign back once you're done. ✅

@nelsonic nelsonic assigned LuchoTurtle and unassigned nelsonic Jan 15, 2023
@nelsonic nelsonic added bug Suspected or confirmed bug (defect) in the code in-progress An issue or pull request that is being worked on by the assigned person and removed in-review Issue or pull request that is currently being reviewed by the assigned person labels Jan 15, 2023
@LuchoTurtle
Copy link
Member Author

The merge should have fixed the problem.
Thanks!

@LuchoTurtle
Copy link
Member Author

AppWeb.API.TimerControllerTest -> API.TimerTest 😉

Just for clarification, I followed a "convention" that appeared on Stack Overflow -> https://stackoverflow.com/questions/42598281/where-to-put-phoenix-controllers-for-app-with-both-web-frontend-and-an-api

But I do agree your change makes more sense 👍

@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 16, 2023
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jan 16, 2023
@nelsonic nelsonic mentioned this pull request Jan 16, 2023
4 tasks
lib/api/item.ex 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.

@LuchoTurtle Looks good. Thanks. 👍
Made a couple of notes in: #256 (comment) and opened a couple of new follow-up / side-quest issues: #269 & #268

Merging. :shipit:

@nelsonic
Copy link
Member

Testing the /api of the Review App:

curl -X POST https://mvp-pr-263.fly.dev/api/items -H 'Content-Type: application/json' -d '{"text":"My Awesome item text"}' 

Response:

{"id":1}

Satisfied this is working. ✅

@nelsonic nelsonic merged commit 130d443 into main Jan 16, 2023
@nelsonic nelsonic deleted the api-#256 branch January 16, 2023 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed bug Suspected or confirmed bug (defect) in the code enhancement New feature or enhancement of existing functionality technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants