MF-1290 - Sort Things and Channels by name #1293

manuio · 2020-11-25T15:45:15Z

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

Resolves #1290

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

codecov-io · 2020-11-25T15:50:28Z

Codecov Report

Merging #1293 (8e19f3b) into master (fbba7aa) will decrease coverage by 0.11%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##           master    #1293      +/-   ##
==========================================
- Coverage   64.57%   64.45%   -0.12%     
==========================================
  Files         117      109       -8     
  Lines        7994     7858     -136     
==========================================
- Hits         5162     5065      -97     
+ Misses       2379     2352      -27     
+ Partials      453      441      -12

Impacted Files	Coverage Δ
things/api/things/http/responses.go	`96.22% <ø> (ø)`
things/api/things/http/transport.go	`89.19% <73.33%> (-0.84%)`	⬇️
things/api/things/http/endpoint.go	`98.45% <100.00%> (+0.02%)`	⬆️
things/api/things/http/requests.go	`87.80% <100.00%> (+0.96%)`	⬆️
things/postgres/channels.go	`80.68% <100.00%> (+0.91%)`	⬆️
things/postgres/things.go	`79.71% <100.00%> (+0.29%)`	⬆️
things/redis/streams.go	`85.59% <100.00%> (ø)`
things/service.go	`54.28% <100.00%> (ø)`
authz/api/http/transport.go
authz/api/grpc/requests.go
... 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 fbba7aa...f91f36d. Read the comment docs.

bbokun

LGTM

drasko · 2020-11-25T19:08:59Z

things/postgres/channels.go

@@ -128,7 +128,7 @@ func (cr channelRepository) RetrieveAll(ctx context.Context, owner string, offse
 	}

 	q := fmt.Sprintf(`SELECT id, name, metadata FROM channels
-	      WHERE owner = :owner %s%s ORDER BY id LIMIT :limit OFFSET :offset;`, mq, nq)
+	      WHERE owner = :owner %s%s ORDER BY name LIMIT :limit OFFSET :offset;`, mq, nq)


I am really not sure about this one... Is name even mandatory in our Thing entity?

Name is not mandatory but sorting works for empty strings.

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

dborovcanin · 2020-12-04T09:52:12Z

things/api/logging.go

-		if name != "" {
-			nlog = fmt.Sprintf("with name %s ", name)
+		if pm.Name != "" {
+			nlog = fmt.Sprintf("with name %s ", pm.Name)


It's not related to this PR, but can you, please, fix the formatting: nlog = fmt.Sprintf("with name %s", pm.Name) (no space after) and later message := fmt.Sprintf("Method list_things %s for token %s took %s to complete", nlog, token, time.Since(begin)) (%s surrounded with spaces)? Same for the ListChannels.

things/postgres/channels.go

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

dborovcanin · 2020-12-04T16:44:04Z

Please add order to the API response so that we can merge this one.

things/postgres/channels_test.go

things/postgres/things_test.go

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

dborovcanin · 2020-12-08T15:13:03Z

things/openapi.yml

+      schema:
+        type: string
+        default: id
+        format: byte


Byte format indicates base64-encoded binary. You can simply remove it.

dborovcanin · 2020-12-08T15:13:10Z

things/openapi.yml

+      schema:
+        type: string
+        default: desc
+        format: byte


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

darkodraskovic · 2020-12-08T15:41:05Z

things/openapi.yml

@@ -654,8 +654,7 @@ components:
      description: Unique thing identifier.
      in: path
      schema:
-        type: integer
-        minimum: 1
+        type: string


Add here format: uuid according to https://swagger.io/docs/specification/data-models/data-types/

dborovcanin · 2020-12-08T15:45:04Z

things/api/things/http/endpoint_test.go

@@ -663,6 +663,13 @@ func TestListThings(t *testing.T) {
 			url:    fmt.Sprintf("%s?offset=%d&limit=%d", thingURL, 0, 5),
 			res:    data[0:5],
 		},
+		{
+			desc:   "get a list of things ordered by ascendent name",


order by name ascendent
Also, this seems to be descendnet.

dborovcanin · 2020-12-08T15:45:15Z

things/api/things/http/endpoint_test.go

@@ -1425,6 +1432,13 @@ func TestListChannels(t *testing.T) {
 			url:    fmt.Sprintf("%s?offset=%d&limit=%d", channelURL, 0, 6),
 			res:    channels[0:6],
 		},
+		{
+			desc:   "get a list of channels ordered by ascendent name",


dborovcanin · 2020-12-08T15:46:06Z

things/postgres/channels_test.go

+			},
+			size: nameMetaNum,
+		},
+		"retrieve channels sorted by ascendent name": {


...ordered by name ascendent...

dborovcanin · 2020-12-08T15:46:45Z

things/postgres/channels_test.go

+			},
+			size: n,
+		},
+		"retrieve channels sorted by descendent name": {


...ordered by name descendent...

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

darkodraskovic · 2020-12-08T15:59:12Z

things/openapi.yml

@@ -642,15 +647,14 @@ components:
      description: Unique channel identifier.
      in: path
      schema:
-        type: string
+        type: uuid


This is wrong. Not type: uuid, but format: uuid and keep type: string.

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

drasko · 2020-12-08T17:33:03Z

things/api/things/http/endpoint.go

@@ -151,7 +151,7 @@ func listThingsEndpoint(svc things.Service) endpoint.Endpoint {
 			return nil, err
 		}

-		page, err := svc.ListThings(ctx, req.token, req.offset, req.limit, req.name, req.metadata)
+		page, err := svc.ListThings(ctx, req.token, req.pageMeta)


I do not like the fact that previously it was called pageMetadata. I think we should stay consistent. I would advise putting pageMetadata here as well.

drasko · 2020-12-08T17:33:12Z

things/api/things/http/endpoint.go

@@ -355,7 +357,7 @@ func listChannelsEndpoint(svc things.Service) endpoint.Endpoint {
 			return nil, err
 		}

-		page, err := svc.ListChannels(ctx, req.token, req.offset, req.limit, req.name, req.metadata)
+		page, err := svc.ListChannels(ctx, req.token, req.pageMeta)


drasko · 2020-12-08T17:33:31Z

things/api/things/http/requests.go

 }

 func (req *listResourcesReq) validate() error {
 	if req.token == "" {
 		return things.ErrUnauthorizedAccess
 	}

-	if req.limit == 0 || req.limit > maxLimitSize {
+	if req.pageMeta.Limit == 0 || req.pageMeta.Limit > maxLimitSize {


Here probably also

drasko · 2020-12-08T17:34:38Z

things/service_test.go

-			err:      nil,
-			metadata: meta,
+			token: token,
+			pageMeta: things.PageMetadata{


pageMetadata probably?

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

drasko

LGTM

Good remark. This was resolved.

MF-1290 - Sort Things and Channels by name

640e75d

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

manuio requested a review from a team as a code owner November 25, 2020 15:45

bbokun previously approved these changes Nov 25, 2020

View reviewed changes

drasko requested changes Nov 25, 2020

View reviewed changes

blokovi previously approved these changes Dec 3, 2020

View reviewed changes

manuio added 2 commits December 4, 2020 10:18

Add sort HTTP parameter and use PageMetadata to pass filters

d619d90

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

Fix merge conflicts

d8a59f8

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

manuio dismissed stale reviews from blokovi and bbokun via d8a59f8 December 4, 2020 09:25

manuio added 2 commits December 4, 2020 10:31

Fix channels conn list

7ac6147

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

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

2d48c77

dborovcanin reviewed Dec 4, 2020

View reviewed changes

things/postgres/channels.go Outdated Show resolved Hide resolved

manuio added 3 commits December 4, 2020 11:03

Fix logs

8da41b3

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

Replace sort naming by order

95f7132

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

Add order in responses

ee82bde

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

dborovcanin reviewed Dec 4, 2020

View reviewed changes

things/postgres/channels_test.go Outdated Show resolved Hide resolved

things/postgres/things_test.go Outdated Show resolved Hide resolved

manuio added 5 commits December 7, 2020 12:15

Improve tests

2609865

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

Improve order tests

3dfb813

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

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

54b4cfc

Improve getNameQuery func

704d920

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

Fix commits

a7c1434

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

dborovcanin previously approved these changes Dec 8, 2020

View reviewed changes

Add name-asc and name-desc

2181fe1

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

manuio dismissed dborovcanin’s stale review via 2181fe1 December 8, 2020 12:16

manuio added 3 commits December 8, 2020 13:18

Fix typo

4f8b80b

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

Add dir query parameter

77be23a

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

Fix openapi

7b33b33

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

Fix typo

d31d0eb

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

dborovcanin reviewed Dec 8, 2020

View reviewed changes

Fix openapi format

a824360

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

darkodraskovic reviewed Dec 8, 2020

View reviewed changes

dborovcanin requested changes Dec 8, 2020

View reviewed changes

Fix test messages and openapi.yml

e988014

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

darkodraskovic previously requested changes Dec 8, 2020

View reviewed changes

manuio and others added 4 commits December 8, 2020 17:01

Fix openapi uuid format

0552cc3

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

Check query parameters in endpoint layer

8e19f3b

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

Merge branch 'master' into order

f91f36d

Add asc endpoint tests

63dffe7

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

drasko requested changes Dec 8, 2020

View reviewed changes

Rename pageMeta into pageMetadata

d7abd95

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

dborovcanin approved these changes Dec 8, 2020

View reviewed changes

drasko approved these changes Dec 8, 2020

View reviewed changes

drasko merged commit 3653e6b into absmach:master Dec 8, 2020

manuio deleted the order branch February 23, 2022 12:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-1290 - Sort Things and Channels by name #1293

MF-1290 - Sort Things and Channels by name #1293

manuio commented Nov 25, 2020

codecov-io commented Nov 25, 2020 •

edited

Loading

bbokun left a comment

drasko Nov 25, 2020

manuio Nov 25, 2020

dborovcanin Dec 4, 2020

dborovcanin commented Dec 4, 2020

dborovcanin Dec 8, 2020

dborovcanin Dec 8, 2020

darkodraskovic Dec 8, 2020

dborovcanin Dec 8, 2020

dborovcanin Dec 8, 2020

dborovcanin Dec 8, 2020

dborovcanin Dec 8, 2020

darkodraskovic Dec 8, 2020

drasko Dec 8, 2020

drasko Dec 8, 2020

drasko Dec 8, 2020

drasko Dec 8, 2020

drasko left a comment

MF-1290 - Sort Things and Channels by name #1293

MF-1290 - Sort Things and Channels by name #1293

Conversation

manuio commented Nov 25, 2020

codecov-io commented Nov 25, 2020 • edited Loading

Codecov Report

bbokun 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

dborovcanin commented Dec 4, 2020

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

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

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

drasko left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 25, 2020 •

edited

Loading