You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/06/02 04:52:54 UTC
[GitHub] [trafficcontrol] rimashah25 opened a new pull request #5904: Improvement/git 5451
rimashah25 opened a new pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904
<!--
************ STOP!! ************
If this Pull Request is intended to fix a security vulnerability, DO NOT submit it! Instead, contact
the Apache Software Foundation Security Team at security@trafficcontrol.apache.org and follow the
guidelines at https://www.apache.org/security/ regarding vulnerability disclosure.
-->
## What does this PR (Pull Request) do?
<!-- Explain the changes you made here. If this fixes an Issue, identify it by
replacing the text in the checkbox item with the Issue number e.g.
- [x] This PR fixes #9001 OR is not related to any Issue
^ This will automatically close Issue number 9001 when the Pull Request is
merged (The '#' is important).
Be sure you check the box properly, see the "The following criteria are ALL
met by this PR" section for details.
-->
- [ ] This PR fixes #REPLACE_ME OR is not related to any Issue <!-- You can check for an issue here: https://github.com/apache/trafficcontrol/issues -->
## Which Traffic Control components are affected by this PR?
<!-- Please delete all components from this list that are NOT affected by this
Pull Request. Also, feel free to add the name of a tool or script that is
affected but not on the list.
Additionally, if this Pull Request does NOT affect documentation, please
explain why documentation is not required. -->
- CDN in a Box
- Documentation
- Grove
- Traffic Control Client <!-- Please specify which; e.g. 'Python', 'Go', 'Java' -->
- Traffic Monitor
- Traffic Ops
- Traffic Ops ORT
- Traffic Portal
- Traffic Router
- Traffic Stats
- Traffic Vault
- CI tests
## What is the best way to verify this PR?
<!-- Please include here ALL the steps necessary to test your Pull Request. If
it includes tests (and most should), outline here the steps needed to run the
tests. If not, lay out the manual testing procedure and please explain why
tests are unnecessary for this Pull Request. -->
## If this is a bug fix, what versions of Traffic Control are affected?
<!-- If this PR fixes a bug, please list here all of the affected versions - to
the best of your knowledge. It's also pretty helpful to include a commit hash
of where 'master' is at the time this PR is opened (if it affects master),
because what 'master' means will change over time. For example, if this PR
fixes a bug that's present in master (at commit hash '1df853c8'), in v4.0.0,
and in the current 4.0.1 Release candidate (e.g. RC1), then this list would
look like:
- master (1df853c8)
- 4.0.0
- 4.0.1 (RC1)
If you don't know what other versions might have this bug, AND don't know how
to find the commit hash of 'master', then feel free to leave this section
blank (or, preferably, delete it entirely).
-->
## The following criteria are ALL met by this PR
<!-- Check the boxes to signify that the associated statement is true. To
"check a box", replace the space inside of the square brackets with an 'x'.
e.g.
- [ x] <- Wrong
- [x ] <- Wrong
- [] <- Wrong
- [*] <- Wrong
- [x] <- Correct!
-->
- [ ] This PR includes tests OR I have explained why tests are unnecessary
- [ ] This PR includes documentation OR I have explained why documentation is unnecessary
- [ ] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [ ] This PR includes any and all required license headers
- [ ] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
## Additional Information
<!-- If you would like to include any additional information on the PR for
potential reviewers please put it here.
Some examples of this would be:
- Before and after screenshots/gifs of the Traffic Portal if it is affected
- Links to other dependent Pull Requests
- References to relevant context (e.g. new/updates to dependent libraries,
mailing list records, blueprints)
Feel free to leave this section blank (or, preferably, delete it entirely).
-->
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652337108
##########
File path: traffic_portal/app/src/common/modules/form/user/form.user.tpl.html
##########
@@ -25,6 +25,7 @@
<li class="active">{{userName}}</li>
</ol>
<div class="pull-right" role="group" ng-show="!settings.isNew">
+ <button name="viewChangelog" class="btn btn-primary" title="Change Log" ng-click="getChangeLogs(user.username)">View Change Log</button>
Review comment:
Because click handlers should never perform navigation if possible. I can't middle-click a button to open it in a new tab, or right-click -> copy link URL, or any of the other things I expect from links. It also messes with assistive technologies; if you rely on a screen reader to find hyperlinks between pages, this can be annoying to dig through.
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r659105907
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +123,64 @@ 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`
+
+const countQuery = `SELECT count(l.tm_user) FROM log as l`
+
+func getLog(inf *api.APIInfo, days int, limit int) ([]tc.Log, uint64, error) {
+ var count = uint64(0)
+ var whereCount string
+ if _, ok := inf.Params["limit"]; !ok {
+ inf.Params["limit"] = strconv.Itoa(DefaultLogLimit)
+ } else {
+ inf.Params["limit"] = strconv.Itoa(limit)
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ }
+ where, _, 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 where != "" {
+ whereCount = ", tm_user as u\n" + where + " AND l.tm_user = u.id"
+ where = where + " AND " + timeInterval
+ } else {
+ whereCount = where
+ where = "\nWHERE " + timeInterval
+ }
+
+ queryCount := countQuery + whereCount
+ rowCount, err := inf.Tx.NamedQuery(queryCount, queryValues)
+ if err != nil {
+ return nil, count, errors.New("querying log count for a given user: " + err.Error())
+ }
+ for rowCount.Next() {
+ if err = rowCount.Scan(&count); err != nil {
+ return nil, count, errors.New("scanning logs: " + err.Error())
+ }
+ }
+
+ query := selectFromQuery + where + "\n ORDER BY last_updated DESC" + pagination
Review comment:
This means the endpoint no longer supports the `orderby` or `sortOrder` query parameters, so you gotta take them out of the docs
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r665499725
##########
File path: lib/go-tc/users.go
##########
@@ -108,6 +107,12 @@ type User struct {
commonUserFields
}
+// UserV40 contains ChangeLogCount field
Review comment:
Nit: GoDoc missing punctuation
##########
File path: traffic_ops/traffic_ops_golang/user/user.go
##########
@@ -204,7 +206,17 @@ func (this *TOUser) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
groupBy := "\n" + `GROUP BY u.id, r.name, t.name`
orderBy = groupBy + orderBy
- query := this.SelectQuery() + where + orderBy + pagination
+
+ version := inf.Version
+ if version == nil {
+ return nil, nil, fmt.Errorf("TOUsers.Read called with invalid API version: %d.%d", version.Major, version.Minor), http.StatusInternalServerError, nil
Review comment:
Since `version` is nil in here, this line will always segfault trying to dereference `version.Major` and `version.Minor`
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r655561908
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
Ok.. And other filters have `var thing = function`, so was keeping up with codebase. If we are changing in one, we should follow suit with other 3 filters too.
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
kewl.. all changes pushed.
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r650155570
##########
File path: traffic_ops/testing/api/v4/logs_test.go
##########
@@ -46,3 +47,20 @@ 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)
+ }
+ for _, user := range toLogs.Response {
+ if *user.User != "admin" {
Review comment:
this will segfault if Traffic Ops incorrectly returned a response with an entry that had a `null` or undefined `user` property.
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", 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 {
Review comment:
the `WHERE` clause can contain any value that's in the `queryParamsToQueryCols` mapping that's passed to `dbhelpers.BuildWhereAndOrderByAndPagination` (not limited to just `"username"`), so this check could result in a malformed query. Instead, you can just check for `where == ""`, that'll always do the right thing with the four lines below.
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", Checker: nil},
+ }
+ where, orderBy, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
Review comment:
This adds support for the query parameters `username`, `last_updated` (which should be `lastUpdated` to match the returned object property), `orderby`, `sortOrder`, `offset`, and `page` to _all_ API versions. Currently, only `username` is being added to the documentation, and only in the version 4.0 docs. Older versions will need a `.. versionadded:: ATCv6` directive so that people reading the documentation know that although in ATCv6 the new query parameters are available in API versions 1.1, 1.2, 1.3, 1.4, 1.5, 2.0, 3.0, and 3.1, those same API versions won't support them in earlier versions of ATC.
Alternatively, you could check the API version and build the query differently for different versions, thereby adding the support for these to _only_ APIv4 and only update that documentation, but it'll make the code potentially more awkward. It doesn't matter to me which approach you choose, but the documentation should fully and accurately reflect the API's behavior.
##########
File path: traffic_ops/testing/api/v4/logs_test.go
##########
@@ -46,3 +47,20 @@ 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)
+ }
+ for _, user := range toLogs.Response {
+ if *user.User != "admin" {
Review comment:
instead of hard-coding 'admin' it might be better to use `TOSession.UserName`.
##########
File path: traffic_portal/app/src/common/modules/form/user/form.user.tpl.html
##########
@@ -25,6 +25,7 @@
<li class="active">{{userName}}</li>
</ol>
<div class="pull-right" role="group" ng-show="!settings.isNew">
+ <button name="viewChangelog" class="btn btn-primary" title="Change Log" ng-click="getChangeLogs(user.username)">View Change Log</button>
Review comment:
can't this just be a link to `/change-logs?username={{username()}}` instead of a button that does navigation? (`$scope.username` will need to be a function that returns the sanitized username with `encodeURIComponent` for that example to work)
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", 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 pagination == "" {
+ pagination = pagination + fmt.Sprintf("\nLIMIT %v", limit)
+ }
+ query := selectFromQuery + where + orderBy + pagination
+
+ rows, err := inf.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, errors.New("querying logs: " + err.Error())
+ return nil, count, errors.New("querying logs: " + err.Error())
}
ls := []tc.Log{}
for rows.Next() {
l := tc.Log{}
if err = rows.Scan(&l.ID, &l.Level, &l.Message, &l.User, &l.TicketNum, &l.LastUpdated); err != nil {
- return nil, errors.New("scanning logs: " + err.Error())
+ return nil, count, errors.New("scanning logs: " + err.Error())
}
+ count += 1
ls = append(ls, l)
}
- return ls, nil
+ return ls, count, nil
Review comment:
So regarding `summary`'s `count` property, the docs define it to mean:
> _"`count` contains an unsigned integer that defines the total number of results that could possibly be returned given the non-pagination query parameters supplied by the client."_
... which means you can't just count the things you're returning to the client. You have to make a separate query that doesn't include any pagination. Typically, you can build this query by doing
```go
const countQuery = `
SELECT COUNT(*)
FROM logs
`
func get(args...) {
// normal API stuff, getting the actual data etc.
// this 'where' is the one that comes from the dbhelpers function from above
query = countQuery + where
var count uint64
ns, err := inf.Tx.PrepareNamed(query)
if err != nil {
// handle the error
}
// 'queryValues' are, again, as returned by the dbhelpers function
err = inf.Tx.NamedStmt(ns).QueryRow(queryValues).Scan(&count)
if err != nil {
// handle the error
}
// 'count' now holds the appropriate log count
// do whatever else you need to do, write the response etc.
}
```
... but if that sounds like too much work to you, then don't worry about it, you don't need to add a `summary` to the endpoint, that's a totally optional extension for API endpoints.
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
+ .then(function(response) {
+ $scope.changeLogs = response;
+ locationUtils.navigateToPath('/change-logs?username='+userName);
Review comment:
`userName` should be sanitized with [`encodeURIComponent`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent).
##########
File path: traffic_ops/testing/api/v4/logs_test.go
##########
@@ -46,3 +47,20 @@ 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)
+ }
+ for _, user := range toLogs.Response {
+ if *user.User != "admin" {
+ t.Fatalf("incorrect username seen in logs, expected: `admin`, got: %v", *user.User)
+ }
+ }
+ if len(toLogs.Response) <= 0 {
+ t.Fatalf("Get logs by username: incorrect number of logs returned (%d)", len(toLogs.Response))
Review comment:
I don't think these errors need to be fatal. If one logs response entry has the incorrect username, you can still check the others safely. As for the last one, nothing is done afterwards, so it's safe to continue no matter what.
Also, though, I think the `<= 0` check is highly dependent on the order in which the tests run. Like if I do `go test --cfg ./traffic-ops-test.conf` then it'll pass because a bunch of things are done before it gets to the logs tests, but if I want to test only the logs endpoint like `go test --cfg ./traffic-ops-test.conf --run '^TestLogs$'` then I think this will fail because the user hasn't done anything to generate a changelog yet. Can you force it to to generate a changelog entry - preferably one with the session username and one with a different username - either with API calls or direct database access?
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", 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 pagination == "" {
+ pagination = pagination + fmt.Sprintf("\nLIMIT %v", limit)
+ }
Review comment:
I think this should just set the `limit` parameter in `inf.Params` if it's unset, prior to calling `dbhelpers.BuildWhereAndOrderByAndPagination`. That way, the function can validate that it's a valid limit value.
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", Checker: nil},
+ }
+ where, orderBy, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
Review comment:
One advantage of doing the second thing is that you can make some breaking API changes, like removing the annoying default values for `days` and `limit`.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652211651
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
Review comment:
oh.. so change the html button to a link instead of a call?
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r655388894
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
There shouldn't be a blank line between a JSDoc and the thing it documents.
I would feel worse about asking for such a small change if our CI was working, but fortunately (?) it isn't.
Also nit but I don't think you need to do `var Thing = function ...`, I'd be shocked if `function Thing ...` didn't work.
--
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
[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
Posted by 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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r654056858
##########
File path: traffic_portal/app/src/common/modules/form/user/form.user.tpl.html
##########
@@ -25,6 +25,7 @@
<li class="active">{{userName}}</li>
</ol>
<div class="pull-right" role="group" ng-show="!settings.isNew">
+ <a class="btn btn-primary" ng-href="/#!/change-logs?username={{userName}}">View Change Log</a>
Review comment:
Copy!
--
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
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652177150
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
Review comment:
I believe this call to `getChangeLogs` is un-needed. The first thing the [change-logs page does](https://github.com/apache/trafficcontrol/blob/5b6db34a8a2c930aa56ec73249161f26cd876328/traffic_portal/app/src/modules/private/changeLogs/list/index.js#L35) is run a `getChangeLogs`.
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r655633484
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
That's fine. It doesn't matter, functionally.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652172943
##########
File path: traffic_portal/app/src/common/modules/form/user/form.user.tpl.html
##########
@@ -25,6 +25,7 @@
<li class="active">{{userName}}</li>
</ol>
<div class="pull-right" role="group" ng-show="!settings.isNew">
+ <button name="viewChangelog" class="btn btn-primary" title="Change Log" ng-click="getChangeLogs(user.username)">View Change Log</button>
Review comment:
I can but what's wrong in how it is right now?
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r648760413
##########
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:
I was using tabs but somehow it doesn't like it. Anyways I updated the docs by copying what was seen in /servers.rst
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652168805
##########
File path: traffic_ops/testing/api/v4/logs_test.go
##########
@@ -46,3 +47,20 @@ 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)
+ }
+ for _, user := range toLogs.Response {
+ if *user.User != "admin" {
+ t.Fatalf("incorrect username seen in logs, expected: `admin`, got: %v", *user.User)
+ }
+ }
+ if len(toLogs.Response) <= 0 {
+ t.Fatalf("Get logs by username: incorrect number of logs returned (%d)", len(toLogs.Response))
Review comment:
:+1:
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r655635033
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
kewl.. all changes pushed.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652189318
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
+ .then(function(response) {
+ $scope.changeLogs = response;
+ locationUtils.navigateToPath('/change-logs?username='+userName);
Review comment:
ok but why?
--
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
[GitHub] [trafficcontrol] ocket8888 merged pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
ocket8888 merged pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652336421
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
+ .then(function(response) {
+ $scope.changeLogs = response;
+ locationUtils.navigateToPath('/change-logs?username='+userName);
Review comment:
Because if the username contains <kbd>;</kbd>, <kbd>,</kbd>, <kbd>/</kbd>, <kbd>?</kbd>, <kbd>:</kbd>, <kbd>@</kbd>, <kbd>&</kbd>, <kbd>=</kbd>, <kbd>+</kbd>, <kbd>$</kbd>, or <kbd> </kbd> the URL will be parsed incorrectly - or if the username contains any UTF8 codepoints that begin or end with bytes that could be misinterpreted as those characters. Reserved characters and non-ascii code-points *must* be properly encoded in URLs, **especially** when they come from user input, which usernames do. It's a possible attack vector.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r649248620
##########
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:
Yes
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652213470
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", 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 pagination == "" {
+ pagination = pagination + fmt.Sprintf("\nLIMIT %v", limit)
+ }
+ query := selectFromQuery + where + orderBy + pagination
+
+ rows, err := inf.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, errors.New("querying logs: " + err.Error())
+ return nil, count, errors.New("querying logs: " + err.Error())
}
ls := []tc.Log{}
for rows.Next() {
l := tc.Log{}
if err = rows.Scan(&l.ID, &l.Level, &l.Message, &l.User, &l.TicketNum, &l.LastUpdated); err != nil {
- return nil, errors.New("scanning logs: " + err.Error())
+ return nil, count, errors.New("scanning logs: " + err.Error())
}
+ count += 1
ls = append(ls, l)
}
- return ls, nil
+ return ls, count, nil
Review comment:
Given this; https://github.com/apache/trafficcontrol/issues/5451#issuecomment-764983956, above suggested changed doesn't make sense.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652165936
##########
File path: traffic_ops/testing/api/v4/logs_test.go
##########
@@ -46,3 +47,20 @@ 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)
+ }
+ for _, user := range toLogs.Response {
+ if *user.User != "admin" {
+ t.Fatalf("incorrect username seen in logs, expected: `admin`, got: %v", *user.User)
+ }
+ }
+ if len(toLogs.Response) <= 0 {
+ t.Fatalf("Get logs by username: incorrect number of logs returned (%d)", len(toLogs.Response))
Review comment:
So, I am running the `go test` for logs only and without doing anything additional, I am able to see 10 logs for the user since the first statement of the TestLogs() is:
```
WithObjs(t, []TCObj{Roles, Tenants, Users}, func() { // Objs added to create logs when this test is run alone
```
So, the probability that no logs will be present is 0.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652198696
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", Checker: nil},
+ }
+ where, orderBy, pagination, queryValues, errs :=
+ dbhelpers.BuildWhereAndOrderByAndPagination(inf.Params, queryParamsToQueryCols)
Review comment:
I am choosing the first option and thanks for the helpful suggestion
--
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
[GitHub] [trafficcontrol] mitchell852 commented on pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#issuecomment-875107520
@rimashah25 - very nice feature. i think this will help a lot with user cleanup. couple of very minor comments:
Probably want to change that to "Change Log Count" because it's a label.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r665598445
##########
File path: traffic_ops/traffic_ops_golang/user/user.go
##########
@@ -204,7 +206,17 @@ func (this *TOUser) Read(h http.Header, useIMS bool) ([]interface{}, error, erro
groupBy := "\n" + `GROUP BY u.id, r.name, t.name`
orderBy = groupBy + orderBy
- query := this.SelectQuery() + where + orderBy + pagination
+
+ version := inf.Version
+ if version == nil {
+ return nil, nil, fmt.Errorf("TOUsers.Read called with invalid API version: %d.%d", version.Major, version.Minor), http.StatusInternalServerError, nil
Review comment:
oops
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652213470
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", 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 pagination == "" {
+ pagination = pagination + fmt.Sprintf("\nLIMIT %v", limit)
+ }
+ query := selectFromQuery + where + orderBy + pagination
+
+ rows, err := inf.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, errors.New("querying logs: " + err.Error())
+ return nil, count, errors.New("querying logs: " + err.Error())
}
ls := []tc.Log{}
for rows.Next() {
l := tc.Log{}
if err = rows.Scan(&l.ID, &l.Level, &l.Message, &l.User, &l.TicketNum, &l.LastUpdated); err != nil {
- return nil, errors.New("scanning logs: " + err.Error())
+ return nil, count, errors.New("scanning logs: " + err.Error())
}
+ count += 1
ls = append(ls, l)
}
- return ls, nil
+ return ls, count, nil
Review comment:
Given this; https://github.com/apache/trafficcontrol/issues/5451#issuecomment-764983956, above suggested changed doesn't make sense. But I see what you mean, If I had limit=10, the response will show 10 entries but total:50 for given username
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652165936
##########
File path: traffic_ops/testing/api/v4/logs_test.go
##########
@@ -46,3 +47,20 @@ 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)
+ }
+ for _, user := range toLogs.Response {
+ if *user.User != "admin" {
+ t.Fatalf("incorrect username seen in logs, expected: `admin`, got: %v", *user.User)
+ }
+ }
+ if len(toLogs.Response) <= 0 {
+ t.Fatalf("Get logs by username: incorrect number of logs returned (%d)", len(toLogs.Response))
Review comment:
So, I am running the `go test` for logs only and without doing anything additional, I am able to see 10 logs for the user since the first statement of the TestLogs() is:
```
WithObjs(t, []TCObj{Roles, Tenants, Users}, func() { // Objs added to create logs when this test is run alone
```
So, the probability that no logs will be present is 0.
Also, the error should be fatal since I moved this check (to avoid seg fault) before the for loop and if there are no longs, it should quit and not run for looop
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r655388894
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
There shouldn't be a blank line between a JSDoc and the thing it documents.
I would feel worse about asking for such a small change if our CI was working, but fortunately (?) it isn't.
Also nit but I don't think you need to do `var Thing = function ...`, I'd be shocked if `function Thing ...` didn't work.
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
That's fine. It doesn't matter, functionally.
--
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
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652337546
##########
File path: traffic_ops/traffic_ops_golang/logs/log.go
##########
@@ -121,26 +122,51 @@ 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)
+ if _, ok := inf.Params["orderby"]; !ok {
+ inf.Params["orderby"] = "last_updated"
+ }
+
+ queryParamsToQueryCols := map[string]dbhelpers.WhereColumnInfo{
+ "username": dbhelpers.WhereColumnInfo{Column: "u.username", Checker: nil},
+ "last_updated": dbhelpers.WhereColumnInfo{Column: "l.last_updated", 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 pagination == "" {
+ pagination = pagination + fmt.Sprintf("\nLIMIT %v", limit)
+ }
+ query := selectFromQuery + where + orderBy + pagination
+
+ rows, err := inf.Tx.NamedQuery(query, queryValues)
if err != nil {
- return nil, errors.New("querying logs: " + err.Error())
+ return nil, count, errors.New("querying logs: " + err.Error())
}
ls := []tc.Log{}
for rows.Next() {
l := tc.Log{}
if err = rows.Scan(&l.ID, &l.Level, &l.Message, &l.User, &l.TicketNum, &l.LastUpdated); err != nil {
- return nil, errors.New("scanning logs: " + err.Error())
+ return nil, count, errors.New("scanning logs: " + err.Error())
}
+ count += 1
ls = append(ls, l)
}
- return ls, nil
+ return ls, count, nil
Review comment:
that comment was about a possible alternative to this, but apparently there's enough value in it that we're doing it, so what I said on #5451 no longer matters.
--
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
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652213719
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
Review comment:
That's what @ocket8888 is requesting. If it were to stay a call the function would just need to be `locationUtils.navigateToPath('/change-logs?username='+userName)`.
Didn't really process that his comment was functionally the same as this comment.
--
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
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652213719
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
Review comment:
That's what @ocket8888 is requesting. If it were to stay a call the function would just need to be `locationUtils.navigateToPath('/change-logs?username='+userName)`.
I didn't really process that his comment was functionally the same as this comment.
--
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
[GitHub] [trafficcontrol] shamrickus commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
shamrickus commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652213719
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
Review comment:
That's what @ocket8888 is requesting. If it were to stay a call the function would just need to be `locationUtils.navigateToPath('/change-logs?username='+userName)`.
--
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
[GitHub] [trafficcontrol] mitchell852 edited a comment on pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
mitchell852 edited a comment on pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#issuecomment-875107520
@rimashah25 - very nice feature. i think this will help a lot with user cleanup. couple of very minor comments:
Probably want to change that to "Change Log Count" because it's a label. other than that, looks great.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[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
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r652336421
##########
File path: traffic_portal/app/src/common/modules/form/user/FormUserController.js
##########
@@ -37,6 +37,16 @@ var FormUserController = function(user, $scope, $location, formUtils, stringUtil
});
};
+ $scope.changeLogs = [];
+
+ $scope.getChangeLogs = function(userName) {
+ changeLogService.getChangeLogs({ username: userName })
+ .then(function(response) {
+ $scope.changeLogs = response;
+ locationUtils.navigateToPath('/change-logs?username='+userName);
Review comment:
Because if the username contains <kbd>;</kbd>, <kbd>,</kbd>, <kbd>/</kbd>, <kbd>?</kbd>, <kbd>:</kbd>, <kbd>@</kbd>, <kbd>&</kbd>, <kbd>=</kbd>, <kbd>+</kbd>, <kbd>$</kbd>, or <kbd> </kbd> the URL will be parsed incorrectly - or if the username contains any UTF8 codepoints that begin or end with bytes that could be misinterpreted as those characters. Reserved characters and non-ascii code-points *must* be properly encoded in URLs, **especially** when they come from user input, which usernames do. It's a possible attack vector.
--
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
[GitHub] [trafficcontrol] rimashah25 commented on a change in pull request #5904: Add change log count to user response payload and username as query param in logs API
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5904:
URL: https://github.com/apache/trafficcontrol/pull/5904#discussion_r655561908
##########
File path: traffic_portal/app/src/common/filters/EncodeURIComponentFilter.js
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ * This is a factory function for an AngularJS filter meant to allow easy
+ * sanitization of interpolated input into URL query strings.
+ *
+ * @example
+ *
+ * <a ng-href="https://example.test/something?foo={{someString | encodeURIComponent}}">
+ * A safe link
+ * </a>
+ *
+ * @returns {(input: string) => string} A filter that sanitizes input text for safe insertion into URL query strings.
+ */
+
+var EncodeURIComponentFilter = function() {
Review comment:
Ok.. And other filters have `var thing = function`, so was keeping up with codebase. If we are changing in one, we should follow suit with other 3 filters too.
--
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