Check mail for missing attachment #6172

max65482 · 2022-03-31T19:11:55Z

This PR sketches a solution to awkward problems that I sometimes encounter before my first coffee: I misspell the name of the email recipient in the salutation (Dear XXX) or I mention an attachment, but forget to attach something.

Before sending an email, the app now searches for these accidents and warns the user.

If there is interest, I can refactor and give the code presentable quality. Otherwise, feel free to close this.

welcome · 2022-03-31T19:11:56Z

Thanks for opening your first pull request in this repository! ✌️

ChristophWurst

I like the feature, e.g. to warn for the possibly missing attachments.

Translation will be tricky, because there are one or multiple words in each language (and variants of them, think: singular vs plural). For HTML messages we need to saveguard that a translation doesn't match a HTML tag or attribute, because that would show false warnings.

The editor is based on CKEditor. We could check if there is anything existing, like a module built in or a third party module that we could add.

ChristophWurst · 2022-08-19T15:02:19Z

src/components/NewMessageModal.vue

+					if (warnings !== "") {
+						warnings += ", "
+					}
+					warnings += "missing attachments"


this needs translation

@max65482 could you address this please?

I will be looking at this PR again soon (with warnings only about missing attachments, not salutations for now).
(If you are referring to the translation: At this time I feel unable to address this.)

max65482 · 2023-01-19T21:23:56Z

Translation will be tricky, because there are one or multiple words in each language (and variants of them, think: singular vs plural).

For simplicity, we should assume that the singular word is always part of the plural word.

For HTML messages we need to saveguard that a translation doesn't match a HTML tag or attribute, because that would show false warnings.

toPlain(data.body) should be enough to get rid of HTML tags etc., right?

max65482 · 2023-01-19T21:25:13Z

@ChristophWurst Can you give it another look?

ChristophWurst

Looks good otherwise codewise

Are users able to send despite the error?

ChristophWurst · 2023-02-23T13:17:59Z

src/components/NewMessageModal.vue

+						break
+					}
+					if (line.includes(wordAttachment) || line.includes(wordAttached)) {
+						attachmentMissing = true


remove the variable and just throw here

ChristophWurst · 2023-02-23T13:18:18Z

src/components/NewMessageModal.vue

+				var lines = toPlain(data.body).value.toLowerCase().split('\n')
+				var wordAttachment = t('mail', 'attachment').toLowerCase()
+				var wordAttached = t('mail', 'attached').toLowerCase()


replace var with const

max65482 · 2023-02-23T19:02:22Z

@ChristophWurst I rebased and stumbled across changes that came with this commit.

mail/src/components/NewMessageModal.vue

Lines 6 to 32 in 41b17c7

    
           <EmptyContent v-if="error" 
        
           	:title="t('mail', 'Error sending your message')" 
        
           	class="centered-content" 
        
           	role="alert"> 
        
           	<p>{{ error }}</p> 
        
           	<template #action> 
        
           		<ButtonVue type="tertiary" @click="state = STATES.EDITING"> 
        
           			{{ t('mail', 'Go back') }} 
        
           		</ButtonVue> 
        
           		<ButtonVue type="tertiary" @click="onSend"> 
        
           			{{ t('mail', 'Retry') }} 
        
           		</ButtonVue> 
        
           	</template> 
        
           </EmptyContent> 
        
           <Loading v-else-if="uploadingAttachments" :hint="t('mail', 'Uploading attachments …')" role="alert" /> 
        
           <Loading v-else-if="sending" 
        
           	:hint="t('mail', 'Sending …')" 
        
           	role="alert" /> 
        
           <EmptyContent v-else-if="warning" :title="t('mail', 'Warning sending your message')" role="alert"> 
        
           	<p>{{ warning }}</p> 
        
           	<ButtonVue type="tertiary" @click="state = STATES.EDITING"> 
        
           		{{ t('mail', 'Go back') }} 
        
           	</ButtonVue> 
        
           	<ButtonVue type="tertiary" @click="onForceSend"> 
        
           		{{ t('mail', 'Send anyway') }} 
        
           	</ButtonVue> 
        
           </EmptyContent>

In particular, onForceSend does not exist in this context. Could you help out?

ChristophWurst · 2023-06-27T18:16:49Z

We had to move some code around for a new feature. Sorry for the confusion.

max65482 · 2023-07-11T20:03:57Z

I still cannot find onForceSend in this modal although it is referenced. This method would be required for the extension. In my understanding this is technical debt.
As I don't feel proficient enough to fix this on my own, I opened #8617. I will continue once this is fixed.

max65482 · 2023-07-17T18:34:58Z

Thanks to #8625 onForceSend exists now.

Now I stumbled across another issue. NewMessageModal seems to use states that aren't defined in the modal:

mail/src/components/NewMessageModal.vue

Line 18 in 8f7fd93

@click="state = STATES.EDITING">

Can someone help out? @GretaD?

GretaD · 2023-07-19T10:28:02Z

Thanks to #8625 onForceSend exists now.

Now I stumbled across another issue. NewMessageModal seems to use states that aren't defined in the modal:

mail/src/components/NewMessageModal.vue

Line 18 in 8f7fd93

@click="state = STATES.EDITING">

Can someone help out? @GretaD?

i am having a look if its a leftover or needs to be declared

GretaD · 2023-07-27T15:37:39Z

Thanks to #8625 onForceSend exists now.

Now I stumbled across another issue. NewMessageModal seems to use states that aren't defined in the modal:

mail/src/components/NewMessageModal.vue

Line 18 in 8f7fd93

@click="state = STATES.EDITING">

Can someone help out? @GretaD?

the pr to remove the leftovers is merged. You can continue now :)

max65482 · 2023-07-31T11:23:07Z

Thanks @GretaD!
PR is ready for review now.

GretaD · 2023-08-07T16:13:20Z

it works as described, but instead of warning it shows an error. I remember seeing a ticket related to implementing warning, but couldnt find it

max65482 · 2023-08-08T17:32:45Z

Yes, as of now there is no distinction between error and warning. E.g. ManyRecipientsError would also show up as an error although it is more of hint. I agree that (ideally) there would be a separate mechanism for warnings.

Speaking about ManyRecipientsError, this might be an opportunity for fixing #6461 as well.

GretaD · 2023-08-09T10:32:12Z

Yes, as of now there is no distinction between error and warning. E.g. ManyRecipientsError would also show up as an error although it is more of hint. I agree that (ideally) there would be a separate mechanism for warnings.

Speaking about ManyRecipientsError, this might be an opportunity for fixing #6461 as well.

this is the ticket that i was looking for. When Anna is back from vacation, im gonna bring this up

GretaD

we can get this in, when we have the warning part done, we can change this into a warning instead of an error.

src/components/NewMessageModal.vue

@@ -362,7 +390,7 @@ export default {
 			}
 		},
 		async onForceSend() {
-			await this.onSend(null, true)
+			await this.onSend(this.cookedComposerData, true)


GretaD · 2023-09-29T14:13:19Z

after rebasing, this pr has issues, having a look why

Signed-off-by: Maximilian Martin <maximilian_martin@gmx.de>

GretaD · 2023-09-29T14:46:07Z

running npm run lint:fix changed a lot of things. I hope i didnt broke something..now looks good

max65482 marked this pull request as draft March 31, 2022 19:12

szaimen added the 3. to review label Apr 16, 2022

ChristophWurst reviewed Aug 19, 2022

View reviewed changes

max65482 marked this pull request as ready for review January 19, 2023 21:26

max65482 requested a review from ChristophWurst February 22, 2023 17:13

ChristophWurst reviewed Feb 23, 2023

View reviewed changes

max65482 force-pushed the feat_mail_checker branch from d7fa8dd to 9a88393 Compare February 23, 2023 18:52

max65482 marked this pull request as draft July 11, 2023 18:56

max65482 force-pushed the feat_mail_checker branch from 0f78724 to ed74ceb Compare July 11, 2023 18:57

max65482 mentioned this pull request Jul 11, 2023

Technical debt: Missing method 'onForceSend' in NewMessageModal.vue #8617

Closed

max65482 changed the title ~~Check mail for common mistakes before sending~~ Check mail for missing attachment Jul 17, 2023

GretaD mentioned this pull request Jul 19, 2023

Declare editing state on new message modal #8637

Merged

max65482 force-pushed the feat_mail_checker branch from ed74ceb to ca3b68c Compare July 31, 2023 11:20

max65482 marked this pull request as ready for review July 31, 2023 11:22

max65482 requested a review from ChristophWurst July 31, 2023 11:22

GretaD approved these changes Sep 12, 2023

View reviewed changes

ChristophWurst reviewed Sep 29, 2023

View reviewed changes

src/components/NewMessageModal.vue

@@ -362,7 +390,7 @@ export default {

}

},

async onForceSend() {

await this.onSend(null, true)

await this.onSend(this.cookedComposerData, true)

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

GretaD force-pushed the feat_mail_checker branch 2 times, most recently from fd9e7a7 to e2f9310 Compare September 29, 2023 13:54

GretaD force-pushed the feat_mail_checker branch from e2f9310 to acbd487 Compare September 29, 2023 14:39

warn about missing attachments

66346e7

Signed-off-by: Maximilian Martin <maximilian_martin@gmx.de>

GretaD force-pushed the feat_mail_checker branch from acbd487 to 66346e7 Compare September 29, 2023 14:45

GretaD merged commit 4298efc into nextcloud:main Oct 2, 2023
28 checks passed

ChristophWurst mentioned this pull request Feb 7, 2024

Attachment reminders when sending a message #5437

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check mail for missing attachment #6172

Check mail for missing attachment #6172

max65482 commented Mar 31, 2022

welcome bot commented Mar 31, 2022

ChristophWurst left a comment

ChristophWurst Aug 19, 2022

ChristophWurst Jan 18, 2023

max65482 Jan 18, 2023

max65482 commented Jan 19, 2023

max65482 commented Jan 19, 2023

ChristophWurst left a comment

ChristophWurst Feb 23, 2023

ChristophWurst Feb 23, 2023

max65482 commented Feb 23, 2023

ChristophWurst commented Jun 27, 2023

max65482 commented Jul 11, 2023

max65482 commented Jul 17, 2023

GretaD commented Jul 19, 2023

GretaD commented Jul 27, 2023

max65482 commented Jul 31, 2023

GretaD commented Aug 7, 2023

max65482 commented Aug 8, 2023

GretaD commented Aug 9, 2023

GretaD left a comment

This comment was marked as resolved.

This comment was marked as resolved.

GretaD commented Sep 29, 2023

GretaD commented Sep 29, 2023

Check mail for missing attachment #6172

Check mail for missing attachment #6172

Conversation

max65482 commented Mar 31, 2022

welcome bot commented Mar 31, 2022

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst Aug 19, 2022

Choose a reason for hiding this comment

ChristophWurst Jan 18, 2023

Choose a reason for hiding this comment

max65482 Jan 18, 2023

Choose a reason for hiding this comment

max65482 commented Jan 19, 2023

max65482 commented Jan 19, 2023

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst Feb 23, 2023

Choose a reason for hiding this comment

ChristophWurst Feb 23, 2023

Choose a reason for hiding this comment

max65482 commented Feb 23, 2023

ChristophWurst commented Jun 27, 2023

max65482 commented Jul 11, 2023

max65482 commented Jul 17, 2023

GretaD commented Jul 19, 2023

GretaD commented Jul 27, 2023

max65482 commented Jul 31, 2023

GretaD commented Aug 7, 2023

max65482 commented Aug 8, 2023

GretaD commented Aug 9, 2023

GretaD left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

GretaD commented Sep 29, 2023

GretaD commented Sep 29, 2023