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

add sorting capabilities to mail #8231

Merged
merged 1 commit into from
Nov 8, 2023
Merged

add sorting capabilities to mail #8231

merged 1 commit into from
Nov 8, 2023

Conversation

hamza221
Copy link
Contributor

@hamza221 hamza221 commented Mar 15, 2023

Fixes #679

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

@ChristophWurst
Copy link
Member

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

@ChristophWurst
Copy link
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
Copy link
Contributor Author

hamza221 commented Jul 21, 2023

@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
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.

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.

@@ -155,7 +158,8 @@ public function findMessagesGlobally(IUser $user,
}

return $this->messageMapper->findByIds($user->getUID(),
$this->getIdsGlobally($user, $query, $limit)
$this->getIdsGlobally($user, $query, $limit),
'newest'
Copy link
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
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
Copy link
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
Copy link
Contributor

GretaD commented Oct 25, 2023

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
Contributor Author

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

@hamza221
Copy link
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
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
Copy link
Member

ChristophWurst commented Nov 7, 2023

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

src/store/actions.js Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
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
Copy link
Member

ChristophWurst commented Nov 8, 2023

  • 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.

src/store/actions.js Outdated Show resolved Hide resolved
@ChristophWurst
Copy link
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
Member

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

@hamza221
Copy link
Contributor Author

hamza221 commented Nov 8, 2023

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
Member

Frontend and backend tests still need some care

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

@hamza221
Copy link
Contributor Author

hamza221 commented Nov 8, 2023

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

@hamza221
Copy link
Contributor Author

hamza221 commented Nov 8, 2023

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

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

Woah. How?

@hamza221
Copy link
Contributor Author

hamza221 commented Nov 8, 2023

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

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.

Showtime 😎

@ChristophWurst ChristophWurst merged commit d383181 into main Nov 8, 2023
32 checks passed
@ChristophWurst ChristophWurst deleted the feat/mail-sorting branch November 8, 2023 20:49
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