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

AI admin settings #39567

Merged
merged 24 commits into from
Aug 2, 2023
Merged

AI admin settings #39567

merged 24 commits into from
Aug 2, 2023

Conversation

marcelklehr
Copy link
Member

@marcelklehr marcelklehr commented Jul 25, 2023

Summary

Adds an admin settings section for AI settings, allowing admins to specify the precedence of machine translation providers, set the speech-to-text provider to use and select which text processing provider to use for which task.

image

Checklist

switch($key) {
case 'ai.textprocessing_provider_preferences':
// fill $value with $defaultValue values
$value = array_merge($defaultValue, $value);

Check notice

Code scanning / Psalm

PossiblyInvalidArgument Note

Argument 1 of array_merge expects array<array-key, mixed>, but possibly different type array<class-string<OCP\TextProcessing\ITaskType>|int<0, max>, class-string<OCP\TextProcessing\IProvider<OCP\TextProcessing\ITaskType>>|class-string<OCP\Translation\ITranslationProvider>>|class-string<OCP\SpeechToText\ISpeechToTextProvider>|null provided
$value = array_merge($defaultValue, $value);
break;
case 'ai.translation_provider_preferences':
$value += array_diff($defaultValue, $value); // Add entries from $defaultValue that are not in $value to the end of $value

Check notice

Code scanning / Psalm

PossiblyInvalidArgument Note

Argument 1 of array_diff expects array<array-key, mixed>, but possibly different type array<class-string<OCP\TextProcessing\ITaskType>|int<0, max>, class-string<OCP\TextProcessing\IProvider<OCP\TextProcessing\ITaskType>>|class-string<OCP\Translation\ITranslationProvider>>|class-string<OCP\SpeechToText\ISpeechToTextProvider>|null provided
@susnux
Copy link
Contributor

susnux commented Jul 27, 2023

Nice! But keep in mind vuedragable is not accessible, so there is no way to reorder by using the keyboard, this needs to be done by the user of the component itself.

@szaimen szaimen added this to the Nextcloud 28 milestone Jul 27, 2023
@szaimen szaimen added the 3. to review Waiting for reviews label Jul 27, 2023
@marcelklehr
Copy link
Member Author

@susnux How would you go about adding a11y here?

@susnux
Copy link
Contributor

susnux commented Jul 28, 2023

How would you go about adding a11y here?

For Forms I suggested this one: nextcloud/forms#1532

So basically either implement key event listeners (arrow up / down) or add additional buttons for ordering

@marcelklehr
Copy link
Member Author

image

@marcelklehr
Copy link
Member Author

/compile

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Awesome, tested with a FreePrompt, a Summarize and an Outline TP provider.
The fallback works just fine if a provider fails.

Small string changes requested but all the rest LGTM.

I'm also wondering if all the unrelated stuff belongs here but since these are nice fixes, why not letting them in this.

About accessibility, there could be 2 buttons to move items up and down.

apps/settings/src/components/AdminAI.vue Outdated Show resolved Hide resolved
apps/settings/src/components/AdminAI.vue Outdated Show resolved Hide resolved
@marcelklehr
Copy link
Member Author

About accessibility, there could be 2 buttons to move items up and down.

Already added :) (see screenshot above)

@julien-nc
Copy link
Member

/compile

@julien-nc julien-nc requested a review from Pytal August 1, 2023 10:11
@julien-nc
Copy link
Member

@Pytal Thanks for the review. Your changes have been implemented.
Rebased on master.

@julien-nc julien-nc force-pushed the enh/ai-admin-settings branch 3 times, most recently from 1537edf to 474de78 Compare August 1, 2023 13:32
marcelklehr and others added 20 commits August 2, 2023 12:37
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Co-authored-by: Julien Veyssier <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Co-authored-by: Julien Veyssier <[email protected]>
Signed-off-by: Marcel Klehr <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
Signed-off-by: Julien Veyssier <[email protected]>
@julien-nc
Copy link
Member

Cypress failure is related with user settings.
acceptance-app-files failure is related with a file action menu button.

@julien-nc julien-nc merged commit 114cad3 into master Aug 2, 2023
35 of 37 checks passed
@julien-nc julien-nc deleted the enh/ai-admin-settings branch August 2, 2023 12:55
@kesselb
Copy link
Contributor

kesselb commented Aug 2, 2023

The failing TextProcessingTest looks related:

image

@kesselb
Copy link
Contributor

kesselb commented Aug 2, 2023

Fix: #39684

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

Successfully merging this pull request may close these issues.

6 participants