Skip to content

Update IDV report to support multiple issuers (LG-10875)#9148

Merged
zachmargolis merged 5 commits intomainfrom
margolis-multiple-issuer-reports
Sep 6, 2023
Merged

Update IDV report to support multiple issuers (LG-10875)#9148
zachmargolis merged 5 commits intomainfrom
margolis-multiple-issuer-reports

Conversation

@zachmargolis
Copy link
Contributor

  • Expanding use so we can support a specific one-off request, but seemed easier to expand all reports at once

(tagging Ursula and Ada since sometimes these reports are relevant to those teams, especially the background job version)

- Expanding use so we can support a specific one-off request, but seemed
  easier to expand all reports at once
@zachmargolis zachmargolis requested review from a team September 5, 2023 21:41
changelog: Internal, Reporting, Update funnel reports to accept multiple issuers
name
, properties.user_id AS user_id
| filter properties.service_provider = %{issuer}
| filter properties.service_provider IN %{issuers}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirming: Does using an array here just intrinsically work? Without looking through the parser code I would have guessed we had to .join(', ') here. I'm inferring (based on tests) that it gets evaluated by a smarter parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! It's the call to quote() above that formats it as an array

Copy link
Contributor

@soniaconnolly soniaconnolly left a comment

Choose a reason for hiding this comment

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

LGTM

@zachmargolis zachmargolis merged commit fa4183d into main Sep 6, 2023
@zachmargolis zachmargolis deleted the margolis-multiple-issuer-reports branch September 6, 2023 18:46
@aduth aduth mentioned this pull request Sep 11, 2023
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.

5 participants