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

NOISSUE - Fix Update User #959

Merged
merged 4 commits into from
Nov 20, 2019
Merged

NOISSUE - Fix Update User #959

merged 4 commits into from
Nov 20, 2019

Conversation

manuio
Copy link
Contributor

@manuio manuio commented Nov 20, 2019

Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio requested a review from a team as a code owner November 20, 2019 11:46
@codecov-io
Copy link

codecov-io commented Nov 20, 2019

Codecov Report

Merging #959 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #959   +/-   ##
=======================================
  Coverage   83.75%   83.75%           
=======================================
  Files          75       75           
  Lines        5288     5288           
=======================================
  Hits         4429     4429           
  Misses        590      590           
  Partials      269      269
Impacted Files Coverage Δ
users/service.go 84.61% <100%> (ø) ⬆️

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 52c4d4a...cd03389. Read the comment docs.

func TestUpdateUser(t *testing.T) {
svc := newService()
svc.Register(context.Background(), user)
key, _ := svc.Login(context.Background(), user)
nonExistingKey, _ := svc.Login(context.Background(), nonExistingUser)
Copy link
Contributor

Choose a reason for hiding this comment

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

you cant use login here, you havent registered user, this will fail
to create key

j := jwt.New("secret")
key, _ := j.TemporaryKey(user.Email)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mteodor I agree with you but let's just remove this test. No need to create a valid token for this test.

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Copy link
Contributor

@anovakovic01 anovakovic01 left a comment

Choose a reason for hiding this comment

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

LGTM!

@dborovcanin dborovcanin merged commit 1b427f4 into absmach:master Nov 20, 2019
@manuio manuio deleted the users branch November 20, 2019 14:03
manuio added a commit that referenced this pull request Oct 12, 2020
* NOISSUE - Fix Update User

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Rm duplicated test

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

* Fix typo

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
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.

5 participants