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

MARP-747:Improve the explanatory dialog and pimp the documentation. #77

Conversation

ghost
Copy link

@ghost ghost commented Aug 21, 2024

No description provided.

@ghost ghost requested a review from ivy-rew as a code owner August 21, 2024 09:44
Copy link
Contributor

github-actions bot commented Aug 21, 2024

Test Results

8 tests  +2   8 ✅ +2   12s ⏱️ -1s
2 suites ±0   0 💤 ±0 
2 files   ±0   0 ❌ ±0 

Results for commit 9d92202. ± Comparison against base commit c3c0b7b.

♻️ This comment has been updated with latest results.

…nector into feature/MARP-747-ChatGPT-assistant-Not-possible-to-mark-only-one-word-in-xhtlm-code
@github-actions github-actions bot added the enhancement New feature or request label Aug 23, 2024
Comment on lines +52 to 59
private String sendRequest(String context, String question, boolean includeSystemPrompt) {
WebTarget chat = client.get().path("chat/completions");
ArrayNode arrayNode = JsonNodeFactory.instance.arrayNode();
arrayNode.add(message("system", SYSTEM_PROMPT));
if (includeSystemPrompt) {
arrayNode.add(message("system", SYSTEM_PROMPT));
}
arrayNode.add(message("user", String.format("%s \n\n %s", context, question)));
ObjectNode request = completion().set("messages", arrayNode);
Copy link
Member

Choose a reason for hiding this comment

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

Why no test on this rest-centered feature which is easy to re-produce in junit?

Copy link
Contributor

@ntqdinh-axonivy ntqdinh-axonivy Oct 15, 2024

Choose a reason for hiding this comment

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

Hi @ivy-rew
I would handle what Linh is working on from now. We are so sorry for this delay due to some critical issue regarding to marketplace website.
For the testing, we found no function from openai-assistant were tested.
Could we create a separated test project for open-ai-assistant or include it to the recent project?

Copy link
Member

Choose a reason for hiding this comment

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

HI @ntqdinh-axonivy
Sorry this didn't reach me. Please make sure you'll use the common team chat if you receive no answer for concrete requests. Otherwise request get lost simply by me being on vacation or something alike.

Answering the question however, there are already rest-centric tests which could easily be extended: https://github.com/axonivy-market/openai-connector/blob/master/openai-connector-test/src_test/com/axonivy/connector/openai/test/AiAssistanceTest.java
... also the utility method in here could be refactored and tested very simply within the existing testing project. So I wouldn't introduce a new test-project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for your hint @ivy-rew
I will update the test cases for these changes soon.

Copy link
Member

@ivy-rew ivy-rew left a comment

Choose a reason for hiding this comment

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

Looks good, please keep the test-coverage in mind -> it's always difficult to review PR on stories you weren't involved without seeing tests asserting the expected behavior.

@ntqdinh-axonivy
Copy link
Contributor

Hi @ivy-rew
I have just updated the new test case for the main rest-centered function by cloning some needed functions from chatgpt-assitant to utils class ( to prevent the test class from being messy when more test cases are introduced) with corresponding requests & responses.
Could you have a quick look at it?

Copy link
Member

@ivy-rew ivy-rew left a comment

Choose a reason for hiding this comment

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

Good feature, enhanced generalization of test-dialogs, nice test coverage. ⭐

Comment on lines 75 to 81
// Get Eclipse theme colors
Color bgColor = JFaceResources.getColorRegistry().get(JFacePreferences.CONTENT_ASSIST_BACKGROUND_COLOR);
Color fgColor = JFaceResources.getColorRegistry().get(JFacePreferences.CONTENT_ASSIST_FOREGROUND_COLOR);

// Create CSS with Eclipse colors
String css = String.format("body { background-color: %s; color: %s; font-family: '%s'; }", toHex(bgColor.getRGB()),
toHex(fgColor.getRGB()), JFaceResources.getTextFont().getFontData()[0].getName());
Copy link
Member

Choose a reason for hiding this comment

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

the logic here is getting larger and harder to maintain, rather than adding comments,
I'd extract this CSS setup and it's colors to its own method #getEclipseCss()

- Extract dialog css set up to separated method
@ntqdinh-axonivy ntqdinh-axonivy merged commit 86cd5ca into master Nov 20, 2024
7 checks passed
@ntqdinh-axonivy ntqdinh-axonivy deleted the feature/MARP-747-ChatGPT-assistant-Not-possible-to-mark-only-one-word-in-xhtlm-code branch November 20, 2024 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants