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

22 credential model form #43

Merged
merged 5 commits into from
Mar 28, 2022
Merged

22 credential model form #43

merged 5 commits into from
Mar 28, 2022

Conversation

elias-ba
Copy link
Contributor

This PR closes #22 and #23.

@elias-ba elias-ba requested a review from stuartc March 24, 2022 09:39
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #43 (355dce5) into main (25d5945) will decrease coverage by 7.97%.
The diff coverage is 43.05%.

@@            Coverage Diff             @@
##             main      #43      +/-   ##
==========================================
- Coverage   79.49%   71.51%   -7.98%     
==========================================
  Files          34       42       +8     
  Lines         278      323      +45     
==========================================
+ Hits          221      231      +10     
- Misses         57       92      +35     
Impacted Files Coverage Δ
lib/lightning_web/live/credential_live/edit.ex 0.00% <0.00%> (ø)
...ghtning_web/live/credential_live/form_component.ex 0.00% <0.00%> (ø)
lib/lightning_web/live/credential_live/index.ex 0.00% <0.00%> (ø)
lib/lightning_web/live/credential_live/show.ex 0.00% <0.00%> (ø)
lib/lightning_web/live/job_live/form_component.ex 83.33% <ø> (ø)
lib/lightning_web/router.ex 60.86% <57.14%> (-12.82%) ⬇️
lib/lightning/helpers.ex 88.88% <88.88%> (ø)
lib/lightning/credentials.ex 100.00% <100.00%> (ø)
lib/lightning/credentials/credential.ex 100.00% <100.00%> (ø)
lib/lightning/invocation.ex 100.00% <100.00%> (+4.00%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 25d5945...355dce5. Read the comment docs.

@elias-ba
Copy link
Contributor Author

@stuartc don't review yet. I need to add more tests. mix verify didn't warn me about those tho.

Copy link
Member

@stuartc stuartc left a comment

Choose a reason for hiding this comment

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

I've added a note about why CodeCov thinks coerce_json_field isn't fully covered.

I don't see any live tests for Credentials, I suggest copying dataclip or job live tests and adapting them to Credentials - a big strange as I thought the generator would have created the tests for you (they did for me); perhaps theres a flag we have to use...

name: "Sadio Mane",
stats: "goals:126, teams: Metz, Liverpool"
}
end
Copy link
Member

Choose a reason for hiding this comment

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

In order to cover the cases that CodeCov is flagging, there are three other cases:

  • The key doesn't exist i.e. coerce_json_field(%{"foo" => 1}, "bar")
  • The key does exist and is nil i.e. coerce_json_field(%{"foo" => nil}, "foo")
  • The key does exist and is not a string (any) i.e. coerce_json_field(%{"foo" => 1}, "foo")

Even though CodeCov does think the first two are covered, we aren't specifically testing it. The third example is the one it's flagging.

Could you add examples for the above scenarios, all of them result in a 'unmodified' map.

@stuartc stuartc merged commit ff27f05 into main Mar 28, 2022
@stuartc stuartc deleted the 22-credential-model-form branch March 28, 2022 11:46
@taylordowns2000 taylordowns2000 added this to the Sprint 1 milestone Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a credentials model and form
3 participants