Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SuperAdmin] Add optional word clause to query for providers #1552

Merged
merged 3 commits into from
Feb 26, 2024

Conversation

FaisalIqbal211
Copy link
Contributor

@FaisalIqbal211 FaisalIqbal211 commented Feb 22, 2024

Describe your changes

https://www.loom.com/share/93cb4eba1c65413bace24e66984a6121

Issue ticket number and link

Closes #1547

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested on Chrome and Firefox
  • I have tested on a mobile device
  • I have provided a screenshot or recording of changes in my PR if there were updates to the frontend

@FaisalIqbal211
Copy link
Contributor Author

@ecurrencyhodler I have also added a query for the 'metrics/bounties/count' endpoint to ensure alignment with the "metrics/bounties" endpoints. Additionally, I have provided unit tests for both scenarios: metrics bounties and metrics bounties count.

@@ -227,6 +233,8 @@ func (mh *metricHandler) MetricsBountiesCount(w http.ResponseWriter, r *http.Req
return
}

request.Providers = providers
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't pass the providers to the request variable, get it from the request.Query

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

},
}
// Mock the database call to return bounties for the selected providers
mockDb.On("GetBountiesByDateRange", dateRange, req).Return(bounties).Once()
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to mock this amount of Providers, the you don't have this amount of DB calls in the request handler

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

Copy link
Contributor

@kevkevinpal kevkevinpal left a comment

Choose a reason for hiding this comment

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

I think we need to change this logic to filter by org_uuid

db/metrics.go Outdated
@@ -226,9 +233,16 @@ func (db database) GetBountiesByDateRangeCount(r PaymentDateRange, re *http.Requ
statusQuery = ""
}

var providerCondition string
if len(r.Providers) > 0 {
providerCondition = " AND owner_id IN ('" + strings.Join(r.Providers, "','") + "')"
Copy link
Contributor

Choose a reason for hiding this comment

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

owner_id is for the user pubkey we're looking for org_uuid

here is a reference to the bounty object

OrgUuid string `json:"org_uuid"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kevkevinpal provider is the person who created the bounty. Please check these comments #1547 (comment) and #1547 (comment)

db/metrics.go Outdated
if len(r.Providers) > 0 {
providerCondition = " AND owner_id IN ('" + strings.Join(r.Providers, "','") + "')"
} else {
providerCondition = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

lets initialize the providerCondition with this emptystring value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed

db/metrics.go Outdated
@@ -193,7 +193,14 @@ func (db database) GetBountiesByDateRange(r PaymentDateRange, re *http.Request)
limitQuery = fmt.Sprintf("LIMIT %d OFFSET %d", limit, offset)
}

query := `SELECT * FROM public.bounty WHERE created >= '` + r.StartDate + `' AND created <= '` + r.EndDate + `'`
var providerCondition string
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename this to orgFilter since it matches org_uuid better

db/structs.go Outdated
@@ -611,6 +611,7 @@ type PaymentDateRange struct {
StartDate string `json:"start_date"`
EndDate string `json:"end_date"`
PaymentType PaymentType `json:"payment_type,omitempty"`
Providers []string `json:"providers,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change this to OrgFilter

@elraphty
Copy link
Contributor

@FaisalIqbal211 The providers should be a comma separate string {pubkey},{pubkey} in the URL query, just like the format for languages.

@FaisalIqbal211
Copy link
Contributor Author

@FaisalIqbal211 The providers should be a comma separate string {pubkey},{pubkey} in the URL query, just like the format for languages.

@elraphty Done

@FaisalIqbal211
Copy link
Contributor Author

@elraphty @kevkevinpal Needs your attention at this PR.

}

// Provide multiple provider IDs in the request query parameters
req.URL.RawQuery = "provider=provider1&provider=provider2&provider=provider3"
Copy link
Contributor

Choose a reason for hiding this comment

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

@FaisalIqbal211 you have not updated the test to accept, a comma-separated provider string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elraphty Addressed

@elraphty elraphty merged commit 4479847 into stakwork:master Feb 26, 2024
2 checks passed
@FaisalIqbal211
Copy link
Contributor Author

@ecurrencyhodler I have also added a query for the 'metrics/bounties/count' endpoint to ensure alignment with the "metrics/bounties" endpoints. Additionally, I have provided unit tests for both scenarios: metrics bounties and metrics bounties count.

@ecurrencyhodler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SuperAdmin] Add optional word clause to query for providers
3 participants