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.

Sign up for GitHub

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

Retrying to send fail messages (enh) #7180

Closed
wants to merge 7 commits into from
Closed

Retrying to send fail messages (enh) #7180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
65473be
Add failed messages icon
Sep 2, 2022
ebe8013
Add outbox group sending (failed status)
Sep 6, 2022
9027aae
Add autoretry 5 times on load page
Sep 6, 2022
4ddeba3
Fix linter issues
Sep 6, 2022
fa6bd13
Fixup! https://github.com/nextcloud/mail/pull/7180#pullrequestreview-…
Oct 15, 2022
e3c36fa
Fixup! CSS of countdown
Oct 15, 2022
2e97866
Fixup! CSS, conflicts
Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions src/components/NavigationOutbox.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@
:title="t('mail', 'Outbox')"
:to="to">
<template #icon>
<IconOutbox
<NcLoadingIcon v-if="sending"
class="outbox-sending-icon"
:size="20" />
<IconOutbox v-else
class="outbox-opacity-icon"
:size="20" />
</template>
Expand All @@ -42,13 +45,26 @@
<script>
import { NcAppNavigationItem as AppNavigationItem, NcCounterBubble as CounterBubble } from '@nextcloud/vue'
import IconOutbox from 'vue-material-design-icons/InboxArrowUp'
import NcLoadingIcon from '@nextcloud/vue/dist/Components/NcLoadingIcon'

const RETRY_COUNT = 5
const RETRY_TIMEOUT = 10000

export default {
name: 'NavigationOutbox',
components: {
AppNavigationItem,
CounterBubble,
IconOutbox,
NcLoadingIcon,
},
data() {
return {
retry: true,
sending: false,
attempts: 0,
failedMessaged: [],
}
},
computed: {
count() {
Expand All @@ -60,11 +76,55 @@ export default {
}
},
},
watch: {
attempts() {
if (this.attempts <= RETRY_COUNT && this.retry) {
setTimeout(() => {
this.retrySendMessages()
}, RETRY_TIMEOUT)

} else {

this.retry = false
this.attempts = 0
}
},
},
created() {
Copy link
Member

Choose a reason for hiding this comment

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

this means with every page load we retry sending those messages. I'm not sure if this is how we want it.

shouldn't each message have some kind of retry counter so it's only retried a few times? with the first iteration of the outbox feature we continuously retried sending failed messages. but that caused more trouble than expected. so this was deliberately disabled via #6602.

also see #6540. there are some side effects with blindly resending ever pending message in the outbox.

Copy link
Collaborator Author

@sazanof sazanof Nov 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@ChristophWurst thank you! As you said, we can retry X-times to send messages only if message failed when it sent first time. In other cases, do not retry sending. What di you think?

In addition, if automatic sending is still not needed, can I leave the updated functionality for resending messages manually =)

Copy link
Member

Choose a reason for hiding this comment

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

Let's try with a counter 👍

Copy link
Collaborator Author

@sazanof sazanof Oct 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hello everyone!
Is it better to turn on the counter when the page loads, or to attract something like a permanent storage (localstorage, database)? Although probably the latter has more chances for bugs?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say the counter should be in the backend

this.failedMessaged = this.$store.getters['outbox/getAllMessages'].filter(message => {
return message.failed
})

setTimeout(() => {
this.retrySendMessages()
}, RETRY_TIMEOUT)
},
methods: {
retrySendMessages() {
const promises = []
if (this.sending) {
return
}
this.sending = true
this.attempts++
this.failedMessaged.map(async (message) => {
if (!message.pending) {
promises.push(this.$store.dispatch('outbox/sendMessage', { id: message.id }).catch(() => {
// TODO write pending state
}))
}
return message
})
Promise.all(promises).then(() => {
this.sending = false
})
},
},
}
</script>

<style lang="scss" scoped>
:deep(.counter-bubble__counter) {
::v-deep(.counter-bubble__counter) {
margin-right: 43px;
}
.outbox-opacity-icon {
Expand Down
123 changes: 107 additions & 16 deletions src/components/Outbox.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,42 @@
<OutboxMessageContent />
<!-- List -->
<template #list>
<AppContentList>
<Error
v-if="error"
:error="t('mail', 'Could not open outbox')"
message=""
role="alert" />
<LoadingSkeleton
v-else-if="loading" />
<EmptyMailbox v-else-if="messages.length === 0" />
<OutboxMessageListItem
v-for="message in messages"
v-else
:key="message.id"
:message="message" />
</AppContentList>
<div slot="list" class="header__button">
<div class="outbox-retry">
<div v-if="!sending" class="outbox-retry--info">
{{
n('mail', '{count} failed message', '{count} failed messages', failedMessages.length, {count: failedMessages.length})
}}
</div>
<div v-else class="outbox-retry--info">
{{
n('mail', 'Retrying send message', 'Retrying send {count} messages', failedMessages.length, {count: failedMessages.length})
}}
</div>
<span
class="outbox-retry--btn"
:class="{sending: sending}"
@click="sendFailedMessages">
{{ t('mail', 'Retry') }}
</span>
</div>
<AppContentList>
<Error
v-if="error"
:error="t('mail', 'Could not open outbox')"
message=""
role="alert" />
<LoadingSkeleton
v-else-if="loading" />
<EmptyMailbox v-else-if="messages.length === 0" />
<div v-else class="outbox-container">
<OutboxMessageListItem
v-for="message in messages"
:key="message.id"
:message="message" />
</div>
</AppContentList>
</div>
</template>
</AppContent>
</template>
Expand Down Expand Up @@ -72,6 +93,7 @@ export default {
error: false,
loading: false,
refreshInterval: undefined,
sending: false,
}
},
computed: {
Expand All @@ -88,10 +110,16 @@ export default {
messages() {
return this.$store.getters['outbox/getAllMessages']
},
failedMessages() {
return this.messages.filter((message) => {
return message.failed
})
},
},
created() {
// Reload outbox contents every 60 seconds
this.refreshInterval = setInterval(async () => {
// TODO cancel if sending available?
await this.fetchMessages()
}, 60000)
},
Expand Down Expand Up @@ -120,12 +148,75 @@ export default {

this.loading = false
},
sendFailedMessages() {
if (this.sending) {
return false
}
this.sending = true
const promises = []
this.failedMessages.map(async (msg) => {
// sending imitation
const message = Object.assign({}, msg)
message.pending = true
this.$store.commit('outbox/updateMessage', { message })

promises.push(this.$store.dispatch('outbox/sendMessage', { id: message.id })
.then(() => {
this.$store.commit('outbox/deleteMessage', { message })
})
.catch(() => {
message.pending = false
this.$store.commit('outbox/updateMessage', { message })
}))
return msg
})
Promise.all(promises).then((res) => {
this.sending = false
})

},
},
}
</script>

<style lang="scss" scoped>
:deep(.button-vue--vue-secondary) {
::v-deep(.button-vue--vue-secondary) {
box-shadow: none;
}
.header__button {
display: flex;
flex: 1px 0 0;
flex-direction: column;
height: calc(100vh - var(--header-height));

}

.outbox-retry {
display: flex;
align-items: center;
justify-content: space-between;
border-right: 1px solid var(--color-border);
min-height: 52px;
margin: 3px 0 0 52px;
border-right: 1px solid var(--color-border);
position: relative;

.outbox-retry--info {
margin: 4px;
font-weight: bold;
color: var(--color-primary-light-text);
padding-left: 8px;
}

.outbox-retry--btn {
cursor:pointer;
color: var(--color-primary-element);
font-weight: bold;
padding:0 8px;

&.sending {
opacity: 0.5;
}
}
}
</style>
Loading