Skip to content

add sorting capabilities to mail#8231

Merged
ChristophWurst merged 1 commit into
mainfrom
feat/mail-sorting
Nov 8, 2023
Merged

add sorting capabilities to mail#8231
ChristophWurst merged 1 commit into
mainfrom
feat/mail-sorting

Conversation

@hamza221

@hamza221 hamza221 commented Mar 15, 2023

Copy link
Copy Markdown
Contributor

Fixes #679

  • NcCheckboxRadioSwitch's UI seems to be broken
  • Fix tests

@ChristophWurst

Copy link
Copy Markdown
Member

Why do we need to clear the cache for a changed sort order?

@ChristophWurst

Copy link
Copy Markdown
Member

@hamza221 what is the state of this?

We discussed using the existing concept of envelope lists for sorting as well so that each sorting attribute and order goes into the ID of the list and when the attribute/order is changed a new list is used. This eliminates the need to clear any previous data/cache.

@hamza221

hamza221 commented Jul 21, 2023

Copy link
Copy Markdown
Contributor Author

@ChristophWurst I did try that yesterday and it worked, but this method of loading envelopes on sort order changing works too and I think it's more scalable. can you please give it a test? It works fully now, I'm just cleaning the code and UI and fixing tests

@hamza221
hamza221 force-pushed the feat/mail-sorting branch 2 times, most recently from e7e1784 to 625dd2b Compare July 21, 2023 15:04
@hamza221
hamza221 marked this pull request as ready for review July 21, 2023 15:07

@st3iny st3iny left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorting works.

However, the page freezes when syncing a large mailbox. We investigated it and it seems that all mails are returned by the sync route right away leading in a very large response. The UI freezes for some time when inserting/updating all envelopes at once.

The checkbox radio switch also lacks some context but that is an issue of nc-vue as there are not local overrides.

Comment thread lib/Service/Search/MailSearch.php Outdated
return $this->messageMapper->findByIds($user->getUID(),
$this->getIdsGlobally($user, $query, $limit)
$this->getIdsGlobally($user, $query, $limit),
'newest'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
'newest'
'DESC'

@ChristophWurst

Copy link
Copy Markdown
Member

However, the page freezes when syncing a large mailbox. We investigated it and it seems that all mails are returned by the sync route right away leading in a very large response. The UI freezes for some time when inserting/updating all envelopes at once.

Is that still the case after adjusting the database change logic to understand the current sort direction?

The checkbox radio switch also lacks some context but that is an issue of nc-vue as there are not local overrides.

&

NcCheckboxRadioSwitch's UI seems to be broken

Was this reported upstream and is anyone working on getting this resolved?

@ChristophWurst
ChristophWurst marked this pull request as draft August 4, 2023 12:34
@ChristophWurst

Copy link
Copy Markdown
Member

Front-end freeze with the first sync fixed. The logic for new messages needed the sort order adjustment

ref https://github.com/nextcloud/mail/pull/5495/files#diff-b7813cb4b4ca97c9ade82b289273c11f6ffda6283e960b8cc29a1fffda83e131 for the earlier attempt that had the trick already

@GretaD

GretaD commented Oct 25, 2023

Copy link
Copy Markdown
Contributor

Same small comments:

  1. My UI froze on the first sync. But after a couple of hard refresh, all went back to normal
  2. If I have an envelope open, and I change the sorting, that thread stays open. (not sure if theres a better way for this, but just pointing it out)
  3. Same small design issues that Hamza is aware of

@hamza221

Copy link
Copy Markdown
Contributor Author

@nimishavijay @ChristophWurst would we want the thread to stay open after sort order change?

@hamza221

Copy link
Copy Markdown
Contributor Author

Missing Test updates and Ui fixes but ready for review I hope

@hamza221
hamza221 marked this pull request as ready for review October 27, 2023 18:19
@hamza221
hamza221 requested a review from st3iny October 27, 2023 18:19
@hamza221 hamza221 self-assigned this Oct 27, 2023
@ChristophWurst

Copy link
Copy Markdown
Member

Unfortunately this still doesn't fully work for me

  1. Pagination is broken
  2. A bunch of messages are detected as new at the first sync

@ChristophWurst

ChristophWurst commented Nov 7, 2023

Copy link
Copy Markdown
Member
Uncaught (in promise) TypeError: getters.getEnvelopes(...)[0] is undefined
    syncEnvelopes actions.js:809
    handleHttpAuthErrors sessionExpiryHandler.js:25
    syncEnvelopes actions.js:786
    wrappedActionHandler vuex.esm.js:851
    dispatch vuex.esm.js:516
    boundDispatch vuex.esm.js:406
    sync Mailbox.vue:357
    mailbox Mailbox.vue:96

when I navigate between mailboxes that have not been synced before, e.g. on a fresh account

Comment thread src/store/actions.js Outdated
@ChristophWurst

Copy link
Copy Markdown
Member

Testing in an individual mailbox with some test email seems to work just as expected 😎

The leftover bugs might be specific to the unified inbox and background syncs

@ChristophWurst

ChristophWurst commented Nov 8, 2023

Copy link
Copy Markdown
Member
  • BUG: pagination doesn't work in the priority inbox: "testObj is undefined". Top of the stack trace says where. We only use that function once inside the action fetchNextEnvelopes. The combination of the individual lists must contain undefined values.

Comment thread src/store/actions.js Outdated
@ChristophWurst

Copy link
Copy Markdown
Member

UI wise, I think this is not ideal, at least for my custom theme color:

image

It's hard to tell which of the two options is active.

@ChristophWurst

Copy link
Copy Markdown
Member

That is all I can find that the moment. Otherwise this now works for me 👍 and kudos

@hamza221

hamza221 commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

UI wise, I think this is not ideal, at least for my custom theme color:

image

It's hard to tell which of the two options is active.

Switched to a classic radio 📻
image

@ChristophWurst

Copy link
Copy Markdown
Member

Frontend and backend tests still need some care

👍 otherwise. let's get this in. we'll figure out any remaining issues

@hamza221

hamza221 commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

I don't get why the unit test is failing, It's passing locally and it is called exactly 7 time :/

@hamza221

hamza221 commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Newely merged Pr caused it, after rebase I was able to reproduce and fix

Signed-off-by: hamza221 <hamzamahjoubi221@gmail.com>
@ChristophWurst

Copy link
Copy Markdown
Member

Woah. How?

@hamza221

hamza221 commented Nov 8, 2023

Copy link
Copy Markdown
Contributor Author

Woah. How?

I probably did something wrong during the first rebase, or something wrong with the job. search-priority-body preference was added in my last PR and CI was counting it for some reason

@ChristophWurst ChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Showtime 😎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add possibility to change sort attribute

4 participants