[GitHub] [trafficcontrol] ocket8888 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API

[GitBox <gi...@apache.org>]

ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r648597626



##########
File path: docs/source/api/v4/logs.rst
##########
@@ -92,5 +95,8 @@ Response Structure
 			"user": "admin",
 			"id": 443,
 			"message": "1 delivery services were assigned to test"
-		}
-	]}
+		}],
+		"summary": {
+      "count": 20
+    }
+	}

Review comment:
       You're using space-based indentation here - documentation should use tabs

##########
File path: docs/source/api/v4/logs.rst
##########
@@ -32,18 +32,20 @@ Request Structure
 -----------------
 .. table:: Request Query Parameters
 
-	+-------+----------+------------------------------------------------------+
-	| Name  | Required | Description                                          |
-	+=======+==========+======================================================+
-	| days  | no       | An integer number of days of change logs to return   |
-	+-------+----------+------------------------------------------------------+
-	| limit | no       | The number of records to which to limit the response |
-	+-------+----------+------------------------------------------------------+
+	+----------+----------+------------------------------------------------------+
+	| Name     | Required | Description                                          |
+	+==========+==========+======================================================+
+	| days     | no       | An integer number of days of change logs to return   |
+	+----------+----------+------------------------------------------------------+
+	| limit    | no       | The number of records to which to limit the response |
+	+----------+----------+------------------------------------------------------+
+	| username | no       | A name to which to limit the response too 					 |

Review comment:
       Malformed table

##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,50 @@ func getLastSeenCookie(r *http.Request) (time.Time, bool) {
 	return lastSeen, true
 }
 
-func getLog(tx *sql.Tx, days int, limit int) ([]tc.Log, error) {
-	rows, err := tx.Query(`
+const selectFromQuery = `
 SELECT l.id, l.level, l.message, u.username as user, l.ticketnum, l.last_updated
-FROM "log" as l JOIN tm_user as u ON l.tm_user = u.id
-WHERE l.last_updated > now() - ($1 || ' DAY')::INTERVAL
-ORDER BY l.last_updated DESC
-LIMIT $2
-`, days, limit)
+FROM "log" as l JOIN tm_user as u ON l.tm_user = u.id`
+
+func getLog(inf *api.APIInfo, days int, limit int) ([]tc.Log, uint64, error) {
+	var count = uint64(0)
+
+	queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+		"username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+	}
+	where, orderBy, pagination, queryValues, errs :=
+		dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
+	if len(errs) > 0 {
+		return nil, 0, util.JoinErrs(errs)
+	}
+
+	timeInterval := fmt.Sprintf("l.last_updated > now() - INTERVAL '%v' DAY", days)
+	if _, ok := inf.Params["username"]; ok {
+		where = where + " AND " + timeInterval
+	} else {
+		where = "\nWHERE " + timeInterval
+	}
+	if orderBy == "" {
+		orderBy = orderBy + "\nORDER BY l.last_updated DESC"

Review comment:
       This should be done by setting the `inf.Params` value at `"orderby"` if it isn't already set. That way, `dbhelpers.BuildWhereAndOrderByAndPagination` will build an `ORDER BY` clause for you, if needed.
   
   Also, since this endpoint uses `dbhelpers.BuildWhereAndOrderByAndPagination` it supports all sorting pagination controls that other endpoints support (e.g. `sortOrder`), but that isn't documented.

##########
File path: docs/source/api/v4/users.rst
##########
@@ -90,6 +90,7 @@ Response Structure
 :tenantId:         The integral, unique identifier of the tenant to which this user belongs
 :uid:              A deprecated field only kept for legacy compatibility reasons that used to contain the UNIX user ID of the user - now it is always ``null``
 :username:         The user's username
+:changeLogCount:   The number of change log entries created by a particular user

Review comment:
       I think this would be clearer if you replace "a particular" with "the"

##########
File path: docs/source/api/v4/logs.rst
##########
@@ -57,6 +59,7 @@ Response Structure
 :message:     Log detail about what occurred
 :ticketNum:   Optional field to cross reference with any bug tracking systems
 :user:        Name of the user who made the change
+:summary.count: Associated changelog entries for a given query param

Review comment:
       `summary.count` is not a property of the response object. Check out the `/servers` endpoint documentation to see an example of how that can be unambiguously handled.

##########
File path: traffic_ops/testing/api/v4/logs_test.go
##########
@@ -46,3 +47,15 @@ func GetTestLogsByLimit(t *testing.T) {
 		t.Fatalf("Get logs by limit: incorrect number of logs returned (%d)", len(toLogs.Response))
 	}
 }
+
+func GetTestLogsByUsername(t *testing.T) {
+	opts := client.NewRequestOptions()
+	opts.QueryParameters.Set("username", "admin")
+	toLogs, _, err := TOSession.GetLogs(opts)
+	if err != nil {
+		t.Errorf("error getting logs: %v - alerts: %+v", err, toLogs.Alerts)
+	}
+	if len(toLogs.Response) <= 0 {
+		t.Fatalf("Get logs by username: incorrect number of logs returned (%d)", len(toLogs.Response))
+	}
+}

Review comment:
       Should this verify that only change logs for the requested user are returned?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org