MF-739 - Add ID to the User entity #1152

manuio · 2020-04-30T08:52:34Z

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

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

codecov-io · 2020-04-30T08:57:48Z

Codecov Report

Merging #1152 into master will decrease coverage by 0.30%.
The diff coverage is 92.00%.

@@            Coverage Diff             @@
##           master    #1152      +/-   ##
==========================================
- Coverage   74.56%   74.25%   -0.31%     
==========================================
  Files         102      102              
  Lines        6946     6855      -91     
==========================================
- Hits         5179     5090      -89     
+ Misses       1415     1411       -4     
- Partials      352      354       +2

Impacted Files	Coverage Δ
sdk/go/sdk.go	`96.55% <ø> (+2.26%)`	⬆️
users/api/responses.go	`75.00% <ø> (-5.00%)`	⬇️
users/users.go	`80.48% <ø> (-4.52%)`	⬇️
users/service.go	`84.78% <60.00%> (-9.02%)`	⬇️
things/postgres/init.go	`93.22% <100.00%> (-0.53%)`	⬇️
users/api/endpoint.go	`83.09% <100.00%> (+0.38%)`	⬆️
users/postgres/init.go	`88.23% <100.00%> (-0.66%)`	⬇️
users/postgres/users.go	`53.62% <100.00%> (+1.62%)`	⬆️
... and 6 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 0c016cd...d962fbc. Read the comment docs.

drasko · 2020-04-30T23:19:28Z

users/postgres/init.go

-						password CHAR(60)	  NOT NULL
-					)`,
+					email VARCHAR(254) PRIMARY KEY,
+					password CHAR(60) NOT NULL)`,


Move closing brackets in the next line, to mark clearly the block closing.

drasko · 2020-04-30T23:20:37Z

users/postgres/init.go

 				},
 				Down: []string{"DROP TABLE users"},
 			},
 			{
 				Id: "users_2",
 				Up: []string{
-					`ALTER TABLE IF EXISTS users ADD COLUMN IF NOT EXISTS metadata JSONB`,
+					`ALTER TABLE IF EXISTS users ADD COLUMN IF NOT EXISTS
+					 metadata JSONB`,


No need to break in two lines IMHO.

drasko · 2020-04-30T23:27:27Z

users/uuid/idp.go

@@ -0,0 +1,32 @@
+// Copyright (c) Mainflux


Is the same one used for things (like UUIDv4 generator)? Maybe then can be moved to project root and mutualized between two packages?

I agree, I wanted to propose this also. Especially because there are 3 services that use idp

I'll fix this for other services in a separate PR: https://github.com/mainflux/mainflux/issues/1154

drasko · 2020-04-30T23:28:30Z

things/postgres/init.go

@@ -83,16 +83,14 @@ func migrateDB(db *sqlx.DB) error {
 				Id: "things_2",
 				Up: []string{
 					`ALTER TABLE IF EXISTS things ALTER COLUMN
-					 metadata TYPE JSONB using metadata::text::jsonb
-					`,
+					 metadata TYPE JSONB using metadata::text::jsonb`,


I kinda liked that this comma was in the new line. I am not saying that this is good solution, but I think it would be worth to check best practices when writing SQL strings in the code...

I would follow the standard which is to don't add a new line. We are also following this standard in other services.

@manuio I agree, I just suggested to check where this standard commens and if it widely accepted, not something just invented by us. How other big Go projects handle this problem?

i think that there are 2 versions:

Up: []string{ `ALTER TABLE IF EXISTS channels ALTER COLUMN metadata TYPE JSONB using metadata::text::jsonb`, },

or

Up: []string{` ALTER TABLE IF EXISTS channels ALTER COLUMN metadata TYPE JSONB using metadata::text::jsonb`, },

And I like the one that we are using

I was expecting some research links... And my remark was about comma (,) at the end of the statement, not backticks.

@drasko the , still there

Is it, but it is not in the new line. So this is what I am trying to figure out - should it be in a new line or not.

In this case it's not a good proposition since it doesn't build, Lines are closed with ,

How did it build before then?

Because of the `,

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

drasko

Generally looks, good. I will let others review as well it is importnant, but I think we are close.

drasko · 2020-05-02T15:22:32Z

uuid/idp.go

@@ -0,0 +1,32 @@
+// Copyright (c) Mainflux


Will we call this top leve package uuid? It does not really implement uuid, but rather our IdP...

I do not know, does not bother me much, but I'd like to her form @mainflux/maintainers

This is not an Identity provider.

maybe we should rename it to uuid/uuid.go ?

Make more sense to me @manuio or put it under /pkg its really shared lib

mteodor

LGTM

nmarcetic · 2020-05-05T10:47:32Z

users/postgres/users.go

@@ -71,7 +71,7 @@ func (ur userRepository) UpdateUser(ctx context.Context, user users.User) error
 }

 func (ur userRepository) RetrieveByID(ctx context.Context, email string) (users.User, error) {


If its RetrieveByID why param is email ?

@nmarcetic because in Users service we use email as unique ID. I can maybe rename the function

Yea because we didn't have ID but now when we introduce ID, this should be changed.

nmarcetic · 2020-05-05T10:52:52Z

uuid/idp.go

@@ -0,0 +1,32 @@
+// Copyright (c) Mainflux


This is not an Identity provider.

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

mteodor · 2020-05-06T09:20:03Z

users/users.go

-	// RetrieveByID retrieves user by its unique identifier (i.e. email).
-	RetrieveByID(context.Context, string) (User, error)
+	// RetrieveByEmail retrieves user by its unique identifier (i.e. email).
+	RetrieveByEmail(ctx context.Context, email string) (User, error)


do we need RetrieveByID too?

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

sprql · 2020-05-08T16:52:23Z

Sorry for off topic, but do you have any plans to use this ID in references in relations instead of email (owner)?

nmarcetic · 2020-05-11T10:58:07Z

Sorry for off topic, but do you have any plans to use this ID in references in relations instead of email (owner)?

This makes sense to me, now when we have ID we should use it as unique resource identification, as we are doing for other entities in the system. @mainflux/maintainers what do you think?

manuio · 2020-05-11T11:05:43Z

@sprql @nmarcetic yes, that's the idea. But we have to think about the backward compatibility of Things and Channels owner.

nmarcetic · 2020-05-11T11:42:14Z

@sprql @nmarcetic yes, that's the idea. But we have to think about the backward compatibility of Things and Channels owner.

Yea it will be a breaking change, make sense. Not sure how we can get backward compatibility, except migration scripts. Which can be tricky for us to do it. Let's think about this

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

codecov-commenter · 2020-06-04T15:04:31Z

Codecov Report

Merging #1152 into master will increase coverage by 0.03%.
The diff coverage is 82.69%.

@@            Coverage Diff             @@
##           master    #1152      +/-   ##
==========================================
+ Coverage   77.25%   77.29%   +0.03%     
==========================================
  Files         102      102              
  Lines        6816     6830      +14     
==========================================
+ Hits         5266     5279      +13     
  Misses       1180     1180              
- Partials      370      371       +1

Impacted Files	Coverage Δ
pkg/sdk/go/sdk.go	`96.55% <ø> (ø)`
users/api/logging.go	`0.00% <0.00%> (ø)`
users/api/metrics.go	`0.00% <0.00%> (ø)`
users/users.go	`80.48% <ø> (ø)`
users/service.go	`84.78% <84.61%> (-1.43%)`	⬇️
things/postgres/init.go	`93.22% <100.00%> (-0.23%)`	⬇️
users/api/endpoint.go	`83.09% <100.00%> (+1.00%)`	⬆️
users/api/requests.go	`73.33% <100.00%> (ø)`
users/api/responses.go	`75.00% <100.00%> (ø)`
users/api/transport.go	`79.56% <100.00%> (ø)`
... and 2 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 7f3e2c1...8af0e06. Read the comment docs.

mteodor

LGTM

* MF-739 - Add ID to the User entity Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Resolve remarks Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Move idp to project root Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Use RetrieveByEmail func and UUIDProvider Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Rm idp.go Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix comment Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Rename UserInfo into ViewUser Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> * Fix ViewUser naming Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>

MF-739 - Add ID to the User entity

4dd6bab

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

manuio requested a review from a team as a code owner April 30, 2020 08:52

drasko requested changes Apr 30, 2020

View reviewed changes

manuio added 3 commits May 1, 2020 08:21

Resolve remarks

c8ed990

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

Merge branch 'master' of https://github.com/mainflux/mainflux into users

c2e4ccf

Move idp to project root

d962fbc

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

drasko reviewed May 2, 2020

View reviewed changes

mteodor previously approved these changes May 4, 2020

View reviewed changes

nmarcetic reviewed May 5, 2020

View reviewed changes

manuio dismissed mteodor’s stale review via 5974de1 May 6, 2020 07:01

manuio added 2 commits May 6, 2020 09:13

Use RetrieveByEmail func and UUIDProvider

5bc64b9

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

Rm idp.go

e0fa715

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

mteodor reviewed May 6, 2020

View reviewed changes

Fix comment

64a79cc

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

mteodor previously approved these changes May 6, 2020

View reviewed changes

drasko added the enhancement label May 12, 2020

drasko modified the milestone: 0.11.0 May 12, 2020

manuio added 2 commits June 3, 2020 21:14

Fix merge conflicts

746d4ff

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

Rename UserInfo into ViewUser

f6e9002

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

manuio dismissed mteodor’s stale review via f6e9002 June 4, 2020 11:27

manuio added 3 commits June 4, 2020 13:47

Fix ViewUser naming

4cb7327

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

Merge branch 'master' of https://github.com/mainflux/mainflux into users

1f2a2af

Fix vendor mproxy 0.2.1 version

8af0e06

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

blokovi approved these changes Jun 4, 2020

View reviewed changes

mteodor approved these changes Jun 4, 2020

View reviewed changes

drasko merged commit 97f8d65 into absmach:master Jun 4, 2020

manuio mentioned this pull request Jun 4, 2020

Database design - add ID to the User entity and use it in Owner fields of other entities #739

Closed

manuio deleted the users branch June 4, 2020 20:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-739 - Add ID to the User entity #1152

MF-739 - Add ID to the User entity #1152

manuio commented Apr 30, 2020

codecov-io commented Apr 30, 2020 •

edited

Loading

drasko Apr 30, 2020

drasko Apr 30, 2020

drasko Apr 30, 2020

manuio May 1, 2020

manuio May 1, 2020 •

edited

Loading

drasko Apr 30, 2020

manuio May 1, 2020

drasko May 1, 2020

manuio May 1, 2020 •

edited

Loading

drasko May 1, 2020

manuio May 1, 2020

drasko May 1, 2020

manuio May 1, 2020 •

edited

Loading

drasko May 1, 2020

manuio May 1, 2020 •

edited

Loading

drasko left a comment

drasko May 2, 2020

nmarcetic May 5, 2020

manuio May 5, 2020

nmarcetic May 5, 2020 •

edited

Loading

mteodor left a comment

nmarcetic May 5, 2020

manuio May 5, 2020 •

edited

Loading

nmarcetic May 5, 2020

nmarcetic May 5, 2020

mteodor May 6, 2020

sprql commented May 8, 2020

nmarcetic commented May 11, 2020

manuio commented May 11, 2020

nmarcetic commented May 11, 2020

codecov-commenter commented Jun 4, 2020

mteodor left a comment

		@@ -71,7 +71,7 @@ func (ur userRepository) UpdateUser(ctx context.Context, user users.User) error
		}

		func (ur userRepository) RetrieveByID(ctx context.Context, email string) (users.User, error) {

MF-739 - Add ID to the User entity #1152

MF-739 - Add ID to the User entity #1152

Conversation

manuio commented Apr 30, 2020

codecov-io commented Apr 30, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio May 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio May 1, 2020 • edited Loading

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmarcetic May 5, 2020 • edited Loading

Choose a reason for hiding this comment

mteodor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuio May 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sprql commented May 8, 2020

nmarcetic commented May 11, 2020

manuio commented May 11, 2020

nmarcetic commented May 11, 2020

codecov-commenter commented Jun 4, 2020

Codecov Report

mteodor left a comment

Choose a reason for hiding this comment

codecov-io commented Apr 30, 2020 •

edited

Loading

manuio May 1, 2020 •

edited

Loading

manuio May 1, 2020 •

edited

Loading

manuio May 1, 2020 •

edited

Loading

manuio May 1, 2020 •

edited

Loading

nmarcetic May 5, 2020 •

edited

Loading

manuio May 5, 2020 •

edited

Loading