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

[OPIK-310] Expose get prompts api #547

Merged
merged 14 commits into from
Nov 5, 2024

Conversation

thiagohora
Copy link
Contributor

@thiagohora thiagohora commented Nov 4, 2024

Details

Find prompts

  • filter by name
  • pagination

Issues

OPIK-310

@thiagohora thiagohora marked this pull request as ready for review November 4, 2024 12:15
@thiagohora thiagohora requested a review from a team as a code owner November 4, 2024 12:15
@thiagohora thiagohora force-pushed the thiagohora/OPIK-310_expose_get_prompts_api branch from 8104d91 to 81dfab6 Compare November 4, 2024 13:57
Copy link
Contributor

@idoberko2 idoberko2 left a comment

Choose a reason for hiding this comment

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

Looks great, had a couple of comments

" ORDER BY id DESC " +
" LIMIT :limit OFFSET :offset ")
@UseStringTemplateEngine
@AllowUnusedBindings
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?

" WHERE workspace_id = :workspace_Id " +
" <if(name)> AND name like concat('%', :name, '%') <endif> ")
@UseStringTemplateEngine
@AllowUnusedBindings
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, because of the string template. If the name is null, it will not render the where clause, and there will be no :name argument.

}

@Test
@DisplayName("when search by name, then return prompt matching name")
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth adding a test for the case of a prompt exists and you search for it with a string that doesn't match the name to see that it is not returned

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

idoberko2
idoberko2 previously approved these changes Nov 4, 2024
Base automatically changed from thiagohora/OPIK-309_exposing_api_contracts to main November 5, 2024 09:39
@thiagohora thiagohora dismissed idoberko2’s stale review November 5, 2024 09:39

The base branch was changed.

@thiagohora thiagohora self-assigned this Nov 5, 2024
@thiagohora thiagohora force-pushed the thiagohora/OPIK-310_expose_get_prompts_api branch from d439827 to ec2175e Compare November 5, 2024 10:42
@thiagohora thiagohora merged commit c1b4f7a into main Nov 5, 2024
7 checks passed
@thiagohora thiagohora deleted the thiagohora/OPIK-310_expose_get_prompts_api branch November 5, 2024 12:23
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.

2 participants