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

feat: Summarize email thread #8653

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Conversation

hamza221
Copy link
Contributor

@hamza221 hamza221 commented Jul 26, 2023

image
image

To Test the setting should be enabled first in admin settings

closes #8508

@hamza221 hamza221 added 3. to review skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills labels Jul 26, 2023
@ChristophWurst ChristophWurst marked this pull request as draft August 1, 2023 11:42
@ChristophWurst ChristophWurst changed the title add thread summary ui feat: Summarize email thread Aug 1, 2023
@ChristophWurst
Copy link
Member

Set to draft because we'll do the full MVP on this branch

@hamza221 hamza221 marked this pull request as ready for review August 1, 2023 15:01
@hamza221
Copy link
Contributor Author

hamza221 commented Aug 1, 2023

FYI before testing nextcloud/server#39658

lib/Controller/ThreadController.php Show resolved Hide resolved
lib/Service/AIIntergrationsService.php Outdated Show resolved Hide resolved
lib/Service/AIIntergrationsService.php Outdated Show resolved Hide resolved
lib/Service/AIIntergrationsService.php Outdated Show resolved Hide resolved
src/components/Thread.vue Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
Member

Text processing APIs only exist with 27.1, 28 and later. This app supports older releases. So we need to find a way to hide this feature when the API is not there.

The AiIntegrationsService could inject ContainerInterface instead and then do something like

try {
   $manager = $this->container->get(IManager::class);
   $manager->magic();
} catch (ContainerError $e) {
   // hide feature
}

@ChristophWurst
Copy link
Member

Not blocking but very nice to have: unit tests for all new classes

@hamza221
Copy link
Contributor Author

hamza221 commented Aug 3, 2023

Not blocking but very nice to have: unit tests for all new classes

I can create an issue and add them in a separate PR? so this gets released asap

@hamza221
Copy link
Contributor Author

hamza221 commented Aug 3, 2023

Any advice on how to make psalm happy for older versions?

@ChristophWurst
Copy link
Member

ChristophWurst commented Aug 4, 2023

Any advice on how to make psalm happy for older versions?

Extend https://github.com/nextcloud/mail/blob/main/tests/psalm-baseline.xml

Extend

mail/psalm.xml

Lines 22 to 41 in ab14bd6

<UndefinedClass>
<errorLevel type="suppress">
<referencedClass name="Doctrine\DBAL\Platforms\MySQLPlatform" />
<referencedClass name="Doctrine\DBAL\Platforms\PostgreSQL94Platform" />
<referencedClass name="Doctrine\DBAL\Platforms\SqlitePlatform" />
<referencedClass name="Doctrine\DBAL\Types\Type" />
<referencedClass name="Doctrine\DBAL\Types\Types" />
<referencedClass name="IPLib\Factory" />
<referencedClass name="IPLib\Address\IPv6" />
<referencedClass name="OC" />
<referencedClass name="OC\Security\CSP\ContentSecurityPolicyNonceManager" />
<referencedClass name="Psr\Http\Client\ClientExceptionInterface" />
<referencedClass name="Symfony\Component\HttpFoundation\IpUtils" />
<referencedClass name="Symfony\Component\Console\Command\Command" />
<referencedClass name="Symfony\Component\Console\Input\InputArgument" />
<referencedClass name="Symfony\Component\Console\Input\InputInterface" />
<referencedClass name="Symfony\Component\Console\Input\InputOption" />
<referencedClass name="Symfony\Component\Console\Output\OutputInterface" />
</errorLevel>
</UndefinedClass>

@hamza221 hamza221 force-pushed the feat/add-thread-summary-frontend branch from 7e1f262 to c38d8a2 Compare August 4, 2023 14:30
appinfo/routes.php Outdated Show resolved Hide resolved
appinfo/routes.php Outdated Show resolved Hide resolved
lib/Service/AiIntegrationsService.php Outdated Show resolved Hide resolved
lib/Service/AiIntegrationsService.php Outdated Show resolved Hide resolved
src/components/LoadingSkeleton.vue Outdated Show resolved Hide resolved
src/components/ThreadSummary.vue Outdated Show resolved Hide resolved
src/components/settings/AdminSettings.vue Outdated Show resolved Hide resolved
src/service/SettingsService.js Outdated Show resolved Hide resolved
src/service/AIIntergrationsService.js Outdated Show resolved Hide resolved
lib/Controller/ThreadController.php Outdated Show resolved Hide resolved
$id,
$messages,
$this->currentUserId,
);
Copy link
Member

Choose a reason for hiding this comment

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

Calling this in a route is risky as it will likely exceed the PHP max execution time. Also, you might want to cache the output. :)

st3iny
st3iny previously requested changes Aug 7, 2023
Copy link
Member

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Tested and works. However, there are some issues in the code.

lib/Controller/ThreadController.php Show resolved Hide resolved
src/components/Thread.vue Outdated Show resolved Hide resolved
src/components/Thread.vue Outdated Show resolved Hide resolved
src/components/Thread.vue Outdated Show resolved Hide resolved
src/service/SettingsService.js Outdated Show resolved Hide resolved
lib/Controller/SettingsController.php Show resolved Hide resolved
lib/Controller/ThreadController.php Outdated Show resolved Hide resolved
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Tested without any llm backend

Ideally we hide the whole feature when there is no text summarization support. That is hide the admin settings and the summary box above threads.

src/service/SettingsService.js Outdated Show resolved Hide resolved
@hamza221 hamza221 force-pushed the feat/add-thread-summary-frontend branch from 3412c1b to 6528bad Compare August 8, 2023 14:46
@ChristophWurst
Copy link
Member

I found myself in a funny situation. I have thread summaries enabled since yesterday. But there is no UI to disable them.

Nextcloud users could run into this as well if they previously had an llm integration enabled but turned it off.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Requesting small changes one last time

Solid PR 👍 😎

}

export const isLlmConfigured = async () => {
const url = generateUrl('/apps/mail/api/settings/llmconfigured')
Copy link
Member

Choose a reason for hiding this comment

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

nit: we can save one request at page load if this retrieval is moved into initial state

@@ -244,6 +244,11 @@ public function index(): TemplateResponse {
$this->config->getAppValue('mail', 'allow_new_mail_accounts', 'yes') === 'yes'
);

$this->initialStateService->provideInitialState(
Copy link
Member

Choose a reason for hiding this comment

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

always set summaries to no if llm apis are not available

lib/Service/AiIntegrationsService.php Outdated Show resolved Hide resolved
Signed-off-by: hamza221 <[email protected]>
@hamza221 hamza221 force-pushed the feat/add-thread-summary-frontend branch from 1857c59 to 0cb4ed5 Compare August 9, 2023 14:16
@ChristophWurst ChristophWurst merged commit 8b23612 into main Aug 9, 2023
28 of 29 checks passed
@ChristophWurst ChristophWurst deleted the feat/add-thread-summary-frontend branch August 9, 2023 16:00
@jakobroehrl
Copy link

image image

To Test the setting should be enabled first in admin settings

closes #8508

@hamza221 I updated the Mail App to 3.4.0-beta.2 but I can't find the "enable thread setting" in my groupware settings.
grafik

Could you help?

@st3iny
Copy link
Member

st3iny commented Aug 17, 2023

The necessary parts in the server are not yet released. You have to be a bit more patient (or install a beta at your own risk).

@ChristophWurst
Copy link
Member

nextcloud/documentation#11025

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release skill:frontend Issues and PRs that require JavaScript/Vue/styling development skills
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Summarize email thread
7 participants