-
Notifications
You must be signed in to change notification settings - Fork 6
Issue 386 opportunity search #430
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
base: main
Are you sure you want to change the base?
Conversation
| search: OpportunitySearchRequest, | ||
| page: int | None = None, | ||
| page_size: int | None = None, | ||
| ) -> OpportunitiesListResponse: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this return OpportunitiesSearchResponse?
widal001
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the general pattern here is looking pretty solid, but had some questions about:
- Extending some of the existing
Client.list_some_items()andClient.list_all_items()rather than re-implementing most of the same logic in new methods. - Whether we wanted to simplify the filter methods we support out of the gate, but keeping them as params on the
search()method for a better DX, or explore other patterns for building complex queries.
| request = { | ||
| "filters": { | ||
| "closeDateRange": { | ||
| "operator": "between", | ||
| "value": {"max": "2025-12-31", "min": "2025-01-01"}, | ||
| }, | ||
| "status": {"operator": "in", "value": ["open", "forecasted"]}, | ||
| "totalFundingAvailableRange": { | ||
| "operator": "between", | ||
| "value": { | ||
| "max": {"amount": "1000000", "currency": "USD"}, | ||
| "min": {"amount": "10000", "currency": "USD"}, | ||
| }, | ||
| }, | ||
| }, | ||
| "pagination": {"page": 1, "pageSize": 10}, | ||
| "search": "local", | ||
| "sorting": {"sortBy": "lastModifiedAt", "sortOrder": "desc"}, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm I'm torn on this.
Part of the reason we want to provide a client SDK is to abstract away some of the complexity of the raw API calls, especially formatting the search and filters correctly.
In some ways, I'd prefer to handle formatting this payload correctly within the Opportunities.search() method rather than offloading it to the SDK user. And instead provide simple optional params in the method. Something like:
Client.opportunities.search(
search="local",
status=[OppStatus.open, OppStatus.forecasted],
)But I also recognize that this could get quite messy if the goal is to support all filters and sorting options.
There are some other patterns we could explore that might be a hybrid of (or alternative to) a totally flattened set of params directly on the search() method:
Something like this, with a semi-flattened OppFilters
Client.opportunities.search(
search="local",
filters=OppFilters(
status=[OppStatus.open, OppStatus.forecasted],
max_close_date="2025-03-01",
min_close_date="2025-01-01,
)
)Or (future PR) create a search builder:
search = (
Client.opportunities
.searchBuilder(search="local")
.addStatuses([OppStatus.open, OppStatus.forecasted])
.addDateRange(min="2025-01-01", max="2025-03-01")
)
search.execute()There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd love to get @DavidDudas-Intuitial's thoughts on this. For this first iteration, do we want to:
- Support a more limited set of filters, but keep them as params in the
search()method for ease of use - Keep the pattern in this PR, expecting SDK users to pass in the full
OpportunitySearchRequestto the method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question @widal001. My guidance would be to keep the scope of this PR narrow with a simple interface as proposed in #386. But let's add an issue to next sprint to define the full-fledged interface to support filters and sorting. Until then, this should be good enough for sdk v0.3:
Client.opportunities.search(
search="local",
status=[OppStatus.open, OppStatus.forecasted],
)
Tthe Opportunities.search() method should hydrate an OpportunitySearchRequest instance and then pass that as an arg to Client.search(). Then Opportunities.search() should return OpportunitiesSearchResponse (not OpportunitiesListResponse).
cc: @jcrichlake
Summary
Client.opportunities.search()#386Changes proposed
Context for reviewers
I set up the opportunity search to take the opportunity search request object since that seemed like a better developer experience for users of the SDK.
You can run the example code for search_opportunity.py to verify the return value of the search
Additional information