mailbox management fix UI#41
mailbox management fix UI#41printminion-co wants to merge 23 commits intofeature/provider_mails_adminfrom
Conversation
fa8bf02 to
dffbbd2
Compare
dffbbd2 to
732b486
Compare
a08601a to
732b486
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds mailbox update functionality to the mail provider administration interface, allowing administrators to edit email localparts and display names for managed mailboxes. The PR implements a comprehensive feature including backend API endpoints, service layer logic, frontend UI components with inline editing, and extensive test coverage.
Changes:
- Added
updateMailboxAPI endpoint with proper authorization and validation - Implemented IONOS-specific mailbox update logic with email uniqueness checks
- Enhanced UI with inline editing capabilities, virtual scrolling for performance, and improved styling
- Added 24 comprehensive unit tests covering edge cases and error scenarios
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/Controller/ExternalAccountsController.php | Added updateMailbox endpoint with input validation, error handling, and display name update logic |
| lib/Provider/MailAccountProvider/IMailAccountProvider.php | Extended interface with updateMailbox method signature |
| lib/Provider/MailAccountProvider/Implementations/IonosProvider.php | Implemented updateMailbox by delegating to facade |
| lib/Provider/MailAccountProvider/Implementations/Ionos/IonosProviderFacade.php | Added updateMailbox with email uniqueness validation and local account synchronization |
| lib/Provider/MailAccountProvider/Implementations/Ionos/Service/Core/IonosAccountMutationService.php | Implemented updateMailboxLocalpart with IONOS API integration and conflict detection |
| lib/Provider/MailAccountProvider/Dto/MailboxInfo.php | Added withMailAppAccountName method for immutable updates |
| lib/Exception/AccountAlreadyExistsException.php | New exception class for handling email conflicts with structured error data |
| lib/Settings/Section/MailProviderAccountsSection.php | Changed icon from mail.svg to mail-dark.svg for better dark mode support |
| appinfo/routes.php | Registered PUT endpoint for mailbox updates |
| src/service/ProviderMailboxService.js | Added updateMailbox service method |
| src/components/provider/mailbox/ProviderMailboxListItem.vue | Implemented inline editing with validation, loading states, and error handling |
| src/components/provider/mailbox/ProviderMailboxAdmin.vue | Integrated VirtualList component and handleUpdate method |
| src/components/provider/mailbox/shared/VirtualList.vue | New component for virtualized list rendering with scroll optimization |
| src/components/provider/mailbox/shared/MailboxListHeader.vue | New header component with debug column support |
| src/components/provider/mailbox/shared/MailboxListFooter.vue | New footer component showing mailbox count and loading state |
| src/components/provider/mailbox/shared/styles.scss | Shared styles for mailbox list components |
| src/components/Envelope.vue | Minor indentation fix (unrelated to main PR) |
| tests/Unit/Controller/ExternalAccountsControllerTest.php | Added 24 comprehensive tests for updateMailbox endpoint |
| tests/Unit/Provider/MailAccountProvider/Implementations/Ionos/Service/Core/IonosAccountMutationServiceTest.php | Updated test setup with queryService dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/Provider/MailAccountProvider/Implementations/Ionos/IonosProviderFacade.php
Outdated
Show resolved
Hide resolved
732b486 to
73e3ff6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2c8cccd to
cbcf0e5
Compare
tanyaka
left a comment
There was a problem hiding this comment.
Review okay if FIX-UP commits make sense to you. Please consider the fixup in ncw-server: IONOS-Productivity/ncw-server#250
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
149b764 to
1a4d379
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <script> | ||
| import debounce from 'debounce' | ||
|
|
||
| // Items to render before and after the visible area | ||
| const bufferItems = 3 | ||
|
|
There was a problem hiding this comment.
debounce is imported as a direct dependency, but it is not listed in this app’s package.json (it currently only comes transitively via @nextcloud/vue). Please add it as an explicit dependency or switch to an already-direct dependency (e.g. lodash debounce) to avoid future builds breaking when transitive deps change.
| return Math.max(0, this.index - bufferItems) | ||
| }, | ||
|
|
||
| shownItems() { | ||
| return Math.ceil((this.tableHeight - this.headerHeight) / this.itemHeight) + bufferItems * 2 |
There was a problem hiding this comment.
bufferItems is stored in data(), but the computed properties use the module-level bufferItems constant instead of this.bufferItems. This is confusing and makes later tuning error-prone; either rely on the constant only (remove from data) or use this.bufferItems consistently.
| return Math.max(0, this.index - bufferItems) | |
| }, | |
| shownItems() { | |
| return Math.ceil((this.tableHeight - this.headerHeight) / this.itemHeight) + bufferItems * 2 | |
| return Math.max(0, this.index - this.bufferItems) | |
| }, | |
| shownItems() { | |
| return Math.ceil((this.tableHeight - this.headerHeight) / this.itemHeight) + this.bufferItems * 2 |
| <th class="header__cell header__cell--linked-user header__cell--large" | ||
| data-cy-mailbox-list-header-linked-user | ||
| scope="col"> | ||
| <span>{{ t('mail', 'Linked User') }}</span> | ||
| </th> |
There was a problem hiding this comment.
header__cell--large is applied but there is no corresponding style in the shared mixins or this component. Consider removing the unused modifier (or adding the missing styles) to avoid dead/misleading CSS classes.
| <td class="footer__cell footer__cell--count"> | ||
| <span aria-describedby="mailbox-count-desc">{{ mailboxCount }}</span> | ||
| <span id="mailbox-count-desc" | ||
| class="hidden-visually"> | ||
| {{ t('mail', 'Scroll to load more rows') }} | ||
| </span> | ||
| </td> |
There was a problem hiding this comment.
The id="mailbox-count-desc" is static. If this component is ever used more than once on a page, it will create duplicate IDs and break aria-describedby linkage. Please generate a per-instance id (e.g. via a computed property using this._uid/a random suffix) and bind both aria-describedby and id to it.
| <tr class="footer"> | ||
| <th scope="row"> | ||
| <span class="hidden-visually">{{ t('mail', 'Total rows summary') }}</span> | ||
| </th> | ||
| <td class="footer__cell footer__cell--loading"> | ||
| <NcLoadingIcon v-if="loading" | ||
| :title="t('mail', 'Loading mailboxes …')" | ||
| :size="32" /> | ||
| </td> | ||
| <td class="footer__cell footer__cell--count"> |
There was a problem hiding this comment.
The footer row renders only 3 cells, while the header/body render more columns (incl. conditional debug + actions). Even with the flex styling, this produces invalid table structure and can confuse assistive technologies. Consider rendering the same number of cells as the header/body (possibly with empty placeholders) or using a single cell with an appropriate colspan that matches the current column count.
| .mailbox-list__row { | ||
| @include styles.row; | ||
| border-bottom: 1px solid var(--color-border); | ||
| transition: background-color 0.1s ease; | ||
|
|
||
| &:last-child { | ||
| border-bottom: none; | ||
| } | ||
|
|
||
| &.editing { | ||
| &:hover { | ||
| background-color: var(--color-background-hover); | ||
| } | ||
|
|
||
| .email-column { | ||
| .email-address { | ||
| font-family: monospace; | ||
| font-size: 14px; | ||
| // Keep sticky cells in sync with hover background | ||
| .row__cell--email, | ||
| .row__cell--actions { | ||
| background-color: var(--color-background-hover); | ||
| } | ||
| } | ||
|
|
||
| .mailbox-field.localpart-field { | ||
| width: 100%; | ||
| max-width: 300px; | ||
| &.row--editing { | ||
| background-color: var(--color-background-hover); | ||
|
|
||
| :deep(.helper-text) { | ||
| .domain-hint { | ||
| color: var(--color-text-lighter); | ||
| font-size: 12px; | ||
| font-family: monospace; | ||
| } | ||
| } | ||
| .row__cell--email, | ||
| .row__cell--actions { | ||
| background-color: var(--color-background-hover); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .user-column { | ||
| .user-info { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 12px; | ||
|
|
||
| .user-icon-placeholder { | ||
| width: 32px; | ||
| height: 32px; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| color: var(--color-text-lighter); | ||
| } | ||
| // Apply cell styles using .row prefix to generate .row__cell selectors | ||
| // (same pattern as UserRow.vue which has .row { @include styles.cell }) | ||
| .row { | ||
| @include styles.cell; | ||
|
|
||
| .user-details { | ||
| .user-display { | ||
| font-weight: 500; | ||
| font-size: 14px; | ||
| &__cell { | ||
| border-bottom: 1px solid var(--color-border); | ||
|
|
There was a problem hiding this comment.
There is a border-bottom set on both the row (.mailbox-list__row) and on every cell (.row__cell), which will typically render as a doubled/thicker separator line (and also reintroduces a border on the last row because only the row’s border is removed). Prefer having the border on either the row or the cells, but not both, and ensure the last row doesn’t keep a bottom border via the remaining rule.
| computed: { | ||
| startIndex() { | ||
| return Math.max(0, this.index - bufferItems) | ||
| }, | ||
|
|
||
| shownItems() { | ||
| return Math.ceil((this.tableHeight - this.headerHeight) / this.itemHeight) + bufferItems * 2 | ||
| }, | ||
|
|
||
| renderedItems() { | ||
| return this.dataSources.slice(this.startIndex, this.startIndex + this.shownItems) | ||
| }, | ||
|
|
||
| tbodyStyle() { | ||
| const isOverScrolled = this.startIndex + this.shownItems > this.dataSources.length | ||
| const lastIndex = this.dataSources.length - this.startIndex - this.shownItems | ||
| const hiddenAfterItems = Math.min(this.dataSources.length - this.startIndex, lastIndex) | ||
| return { | ||
| paddingTop: `${this.startIndex * this.itemHeight}px`, | ||
| paddingBottom: isOverScrolled ? 0 : `${hiddenAfterItems * this.itemHeight}px`, | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
VirtualList introduces non-trivial rendering logic (scroll index calculation, padding math, ResizeObserver behavior), but there are no unit tests added alongside it. Given the repo already has Jest component tests, please add coverage for at least the scroll-to-index calculation and the rendered slice/padding behavior to prevent regressions.
9d32898 to
8fc7571
Compare
…d structure and styling Refactor the mailbox list UI to use a more semantic structure with divs instead of tables, implement sticky headers, and improve styling for better usability and consistency. This change aims to enhance the user experience by providing a clearer layout and more intuitive interaction with mailbox items. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…or better visibility Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…x update Special handling for 404 errors has been added to provide a more helpful message when the mailbox cannot be found. This enhances user experience by guiding users to verify mailbox existence or contact support. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…header and footer components This update introduces a virtualized mailbox list to enhance performance when rendering large datasets. The new structure includes a dedicated header and footer component for better organization and user experience. The mailbox list now efficiently handles scrolling and loading states. Additionally, the styling has been improved for better visual consistency across the mailbox administration interface. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…mailbox list item user layout Signed-off-by: Tatjana Kaschperko Lindt <kaschperko-lindt@strato.de>
…serSessionIndicator Sass is changing behavior for declarations that appear after nested rules to match the CSS spec. Moving background-color above the @media block keeps the existing behavior and eliminates the mixed-decls warning. Signed-off-by: Tatjana Kaschperko Lindt <kaschperko-lindt@strato.de>
…orWebpackPlugin CKEditorWebpackPlugin requires a strategy when multiple JS entry points are present. Using translationsOutputFile targeting mail.js appends CKEditor translations to that bundle, resolving the 'Too many JS assets' error during compilation. Signed-off-by: Tatjana Kaschperko Lindt <kaschperko-lindt@strato.de>
8fc7571 to
aafec79
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <tr class="footer"> | ||
| <th scope="row"> | ||
| <span class="hidden-visually">{{ t('mail', 'Total rows summary') }}</span> | ||
| </th> | ||
| <td class="footer__cell footer__cell--loading"> | ||
| <NcLoadingIcon v-if="loading" | ||
| :title="t('mail', 'Loading mailboxes …')" | ||
| :size="32" /> | ||
| </td> | ||
| <td class="footer__cell footer__cell--count"> | ||
| <span aria-describedby="mailbox-count-desc">{{ mailboxCount }}</span> | ||
| <span id="mailbox-count-desc" | ||
| class="hidden-visually"> | ||
| {{ t('mail', 'Scroll to load more rows') }} | ||
| </span> | ||
| </td> | ||
| </tr> |
There was a problem hiding this comment.
The footer row only contains 3 cells (one th and two td), but the table has at least 4-5 columns (Email Address, Display Name, Linked User, Status (optional), Actions). This mismatch in column count will cause table layout issues and accessibility problems. The footer should either span across all columns using colspan or include empty cells to match the header structure.
| </template> | ||
|
|
||
| <script> | ||
| import debounce from 'debounce' |
There was a problem hiding this comment.
The import statement references 'debounce' package which is not in package.json. The codebase uses 'lodash/fp/debounce.js' for debouncing (as seen in Composer.vue, Thread.vue, TrashRetentionSettings.vue, and others). Change this import to match the project convention.
| import debounce from 'debounce' | |
| import debounce from 'lodash/fp/debounce.js' |
| } | ||
|
|
||
| &__footer { | ||
| inset-inline-start: 0; |
There was a problem hiding this comment.
The footer has position: sticky and inset-inline-start: 0 but is missing the bottom: 0 property. Without this, the footer won't stick to the bottom of the scrollable container. Add bottom: 0 to make the footer properly sticky at the bottom.
| inset-inline-start: 0; | |
| inset-inline-start: 0; | |
| bottom: 0; |
| <div class="status-item" :class="userStatusClass"> | ||
| <component :is="userStatusIcon" :size="16" /> | ||
| <span class="status-label">{{ userStatusLabel }}</span> | ||
| </div> | ||
|
|
||
| <!-- Mail app account status --> | ||
| <!-- Mail app account configured --> | ||
| <div class="status-item" :class="accountStatusClass"> | ||
| <component :is="accountStatusIcon" :size="16" /> | ||
| <span class="status-label">{{ accountStatusLabel }}</span> | ||
| </div> |
There was a problem hiding this comment.
The dynamic component syntax :is="userStatusIcon" and :is="accountStatusIcon" uses computed properties that return string component names. However, Vue 2 does not resolve component names from strings with the :is directive - it needs the actual component object. These dynamic icons will not render correctly. The computed properties should return component objects (e.g., IconCheckCircle) instead of strings (e.g., 'IconCheckCircle').
…yExistsException Replace the fully-qualified class name with a proper use import at the top of the file, consistent with the rest of the codebase. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…odash/fp/debounce.js The debounce npm package is not a direct dependency in package.json. Use lodash/fp/debounce.js which is already a direct dependency and matches the convention used across the rest of the codebase. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
… items fit in viewport When dataSources.length < shownItems the old calculation produced a negative hiddenAfterItems, resulting in a negative paddingBottom. Simplify to Math.max(0, ...) which clamps correctly in all cases. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…revent index jitter Math.round advances the scroll index halfway through a row, causing the rendered slice to shift too early and then jitter back. Math.floor advances the index only after a full row height has been scrolled past. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…se module constant only bufferItems was stored in both the module scope as a constant and in data() as a reactive property, but the computed properties only ever referenced the module-level constant. Remove it from data() to eliminate the confusion. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ck to scroll container bottom The tfoot had position: sticky with inset-inline-start: 0 but was missing bottom: 0, preventing it from anchoring to the bottom of the scrollable table container. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…nt in MailboxListFooter The text said "Scroll to load more rows" implying lazy loading, which was removed. Changed to "Scroll to view more rows" to accurately describe the virtual scroll behaviour. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…h header and body column count The footer row had only 3 cells while the header and body have 4-5. Add a fill cell at the end to produce a structurally valid table and avoid confusing assistive technologies. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…escribedby in MailboxListFooter The static id="mailbox-count-desc" would produce duplicate IDs if the component were mounted more than once. Use this._uid to generate a unique ID per instance. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ge modifier class No style rule exists for this modifier in the shared mixin or the component's own styles. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…ug status column header The title was hard-coded in English. Wrap it in t() so the UI is fully localizable. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…d of strings from status icon computeds Returning string names relies on Vue 2 resolving them against the local registry, which is implicit and fragile. Return the imported component objects directly to make the intent explicit. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…from row cells Both .mailbox-list__row and .row__cell had border-bottom set, producing a doubled separator line. Remove the per-cell border; the row-level border already handles separation, and its :last-child rule correctly removes the final row's border. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
…for screen readers The previous caption explained internal implementation details to screen reader users. Replace with a concise, user-friendly description. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <MailboxListFooter :loading="loading" | ||
| :mailboxes="mailboxes" /> |
There was a problem hiding this comment.
MailboxListFooter is passed :loading="loading", but this entire VirtualList block is only rendered in the v-else branch after v-if="loading" above, so loading will always be false here. Either remove the loading UI/prop from the footer or change the parent logic to keep the list visible during refresh so the footer spinner can actually be shown.
| <MailboxListFooter :loading="loading" | |
| :mailboxes="mailboxes" /> | |
| <MailboxListFooter :mailboxes="mailboxes" /> |
| mounted() { | ||
| const root = this.$el | ||
| const tfoot = this.$refs?.tfoot | ||
| const thead = this.$refs?.thead | ||
|
|
||
| this.resizeObserver = new ResizeObserver(debounce(100, () => { | ||
| this.headerHeight = thead?.clientHeight ?? 0 | ||
| this.tableHeight = root?.clientHeight ?? 0 | ||
| this.onScroll() | ||
| })) | ||
|
|
||
| this.resizeObserver.observe(root) | ||
| this.resizeObserver.observe(tfoot) | ||
| this.resizeObserver.observe(thead) | ||
|
|
There was a problem hiding this comment.
ResizeObserver is used unconditionally here. In environments where it’s not available (notably Jest/jsdom, and potentially older browsers), this will throw at mount time and break the mailbox admin view/tests. Consider guarding with if (typeof ResizeObserver === 'undefined') and falling back to a window resize listener (or at least setting initial headerHeight/tableHeight without observing).
Screencast.from.2026-02-27.14-10-12.mp4
Tests
composer installin order to get new api librarynpm run buildin order to rebuld the frontend code./occ config:app:set --value true --type boolean -- mail ionos_mailconfig_api_allow_insecure