feat(frontend/backend): ensure that the sort fields include the sent …

…and arrival date across all message lists (#1438) * WIP: add a sort parameter to the combined message lists' requests * ensure the sort order correcly applies to the end-user facing list * do not discriminate EWS * extend the sort field for combined pages to have all necessary fields * a fix for the advanced search module * keep the sorting client-side for the search page * fix unit tests
cypht-org · Jan 30, 2025 · dedcfb7 · dedcfb7
1 parent e2ad6a3
commit dedcfb7
Show file tree

Hide file tree

Showing 13 changed files with 155 additions and 67 deletions.
diff --git a/modules/advanced_search/modules.php b/modules/advanced_search/modules.php
@@ -99,7 +99,7 @@ public function process() {
             $msg_list = $this->special_folders_search($mailbox, $flags, $params, $limit);
         } else if ($includeSubfolders) {
             $msg_list = $this->all_folders_search($mailbox, $flags, $params, $limit, $this->folder);
-        } else if (! $mailbox->select_mailbox($this->folder)) {
+        } else if (! $mailbox->select_folder($this->folder)) {
             return;
         } else {
             $msg_list = $this->imap_search($flags, $mailbox, $params, $limit);
@@ -131,7 +131,7 @@ private function special_folders_search($mailbox, $flags, $params, $limit) {
         $msg_list = array();
         foreach ($folders as $folder) {
             $this->folder = $folder;
-            $mailbox->select_mailbox($this->folder);
+            $mailbox->select_folder($this->folder);
             $msgs = $this->imap_search($flags, $mailbox, $params, $limit);
             $msg_list = array_merge($msg_list, $msgs);
         }

diff --git a/modules/core/hm-mailbox.php b/modules/core/hm-mailbox.php
@@ -569,7 +569,7 @@ public function send_message($from, $recipients, $message, $delivery_receipt = f
         }
     }
 
-    protected function select_folder($folder) {
+    public function select_folder($folder) {
         if ($this->is_imap()) {
             if (isset($this->connection->selected_mailbox['name']) && $this->connection->selected_mailbox['name'] == $folder) {
                 return true;

diff --git a/modules/core/js_modules/actions/search.js b/modules/core/js_modules/actions/search.js
@@ -12,8 +12,7 @@ function performSearch(routeParams) {
                         Object.values(response.formatted_message_list).forEach((message) => {
                             Hm_Utils.tbody().append(message['0']);
                         });
-                        // sort by arrival date
-                        Hm_Message_List.sort(4);
+                        Hm_Message_List.sort(getParam('sort') || 'arrival');
                     }
                     Hm_Message_List.check_empty_list();
                 }

diff --git a/modules/core/js_modules/actions/sortCombinedLists.js b/modules/core/js_modules/actions/sortCombinedLists.js
@@ -0,0 +1,15 @@
+async function sortCombinedLists(sortValue) {
+    const url = new URL(window.location.href);
+    url.searchParams.set('sort', sortValue);
+
+    history.pushState(null, null, url.toString());
+    location.next = url.search;
+    const messagesStore = new Hm_MessagesStore(getListPathParam(), getParam('page'));
+    try {
+        await messagesStore.load(true);
+        Hm_Utils.tbody().attr('id', messagesStore.list);
+        display_imap_mailbox(messagesStore.rows, null, messagesStore.list);
+    } catch (error) {
+        Hm_Utils.add_sys_message('Failed to load messages', 'danger');
+    }
+}
diff --git a/modules/core/js_modules/route_handlers.js b/modules/core/js_modules/route_handlers.js
@@ -38,6 +38,7 @@ function applySearchPageHandlers(routeParams) {
     Hm_Message_List.select_combined_view();
     sortHandlerForMessageListAndSearchPage();
     $('.search_reset').on("click", Hm_Utils.reset_search_form);
+    $('.combined_sort').on("change", function() { Hm_Message_List.sort($(this).val()); });
 
     performSearch(routeParams);
 
@@ -86,6 +87,11 @@ function applyMessaleListPageHandlers(routeParams) {
     if (routeParams.list_path === 'github_all') {
         return applyGithubMessageListPageHandler(routeParams);
     }
+
+
+    $('.combined_sort').on("change", function() {
+        sortCombinedLists($(this).val());
+    });
 
     // TODO: Refactor this handler to be more modular(applicable only for the imap list type)
     return applyImapMessageListPageHandlers(routeParams);

diff --git a/modules/core/message_list_functions.php b/modules/core/message_list_functions.php
@@ -132,12 +132,13 @@ function message_list_meta($input, $output_mod) {
  */
 if (!hm_exists('combined_sort_dialog')) {
 function combined_sort_dialog($mod) {
-    $dt_sort = $mod->get('default_sort_order', 'arrival');
-    $sorts = array(
-        '4' => $dt_sort == 'arrival' ? $mod->trans('Arrival Date') : $mod->trans('Sent Date'),
-        '2' => $mod->trans('From'),
-        '3' => $mod->trans('Subject'),
-    );
+    $sorts = [
+        'arrival' => $mod->trans('Arrival Date'),
+        'date' => $mod->trans('Sent Date'),
+        'from' => $mod->trans('From'),
+        'to' => $mod->trans('To'),
+        'subject' => $mod->trans('Subject')
+    ];
 
     $res = '<select name="sort" style="width: 150px" class="combined_sort form-select form-select-sm">';
     foreach ($sorts as $name => $val) {
@@ -342,6 +343,14 @@ function date_callback($vals, $style, $output_mod) {
     return sprintf('<td class="msg_date%s" title="%s">%s<input type="hidden" class="msg_timestamp" value="%s" /></td>', $snooze_class, $output_mod->html_safe(date('r', $vals[1])), $output_mod->html_safe($vals[0]), $output_mod->html_safe($vals[1]));
 }}
 
+function dates_holders_callback($vals) {
+    $res = '<td class="dates d-none">';
+    $res .= '<input type="hidden" name="arrival" class="arrival" value="'. $vals[0] .'" arial-label="Arrival date" />';
+    $res .= '<input type="hidden" name="date" class="date" value="'. $vals[1] .'" arial-label="Sent date" />';
+    $res .= '</td>';
+    return $res;
+}
+
 /**
  * Callback for an icon in a message list row
  * @subpackage core/functions

diff --git a/modules/core/site.css b/modules/core/site.css
@@ -119,9 +119,6 @@ table {
 td {
   vertical-align: top;
 }
-.search_content .combined_sort {
-  display: none;
-}
 .offline {
   display: none;
   cursor: pointer;

diff --git a/modules/core/site.js b/modules/core/site.js
@@ -528,31 +528,22 @@ function Message_List() {
         var aval;
         var bval;
         var sort_result = listitems.sort(function(a, b) {
-            switch (Math.abs(fld)) {
-                case 1:
-                case 2:
-                case 3:
-                    aval = $($('td', a)[Math.abs(fld)]).text().replace(/^\s+/g, '');
-                    bval = $($('td', b)[Math.abs(fld)]).text().replace(/^\s+/g, '');
-                    break;
-                case 4:
-                default:
-                    aval = $('input', $($('td', a)[Math.abs(fld)])).val();
-                    bval = $('input', $($('td', b)[Math.abs(fld)])).val();
-                    break;
-            }
-            if (fld == 4 || fld == -4 || !fld) {
-                if (fld == -4) {
+            const sortField = fld.replace('-', '');
+            if (['arrival', 'date'].includes(sortField)) {
+                aval = new Date($(`input.${sortField}`, $('td.dates', a)).val());
+                bval = new Date($(`input.${sortField}`, $('td.dates', b)).val());
+                if (fld.startsWith('-')) {
                     return aval - bval;
                 }
                 return bval - aval;
             }
-            else {
-                if (fld && fld < 0) {
-                    return bval.toUpperCase().localeCompare(aval.toUpperCase());
-                }
-                return aval.toUpperCase().localeCompare(bval.toUpperCase());
+
+            aval = $(`td.${sortField}`, a).text().replace(/^\s+/g, '');
+            bval = $(`td.${sortField}`, b).text().replace(/^\s+/g, '');
+            if (fld.startsWith('-')) {
+                return bval.toUpperCase().localeCompare(aval.toUpperCase());
             }
+            return aval.toUpperCase().localeCompare(bval.toUpperCase());
         });
         this.sort_fld = fld;
         Hm_Utils.tbody().html('');
@@ -1810,7 +1801,6 @@ var hasLeadingOrTrailingSpaces = function(str) {
 var Hm_Message_List = new Message_List();
 
 function sortHandlerForMessageListAndSearchPage() {
-    $('.combined_sort').on("change", function() { Hm_Message_List.sort($(this).val()); });
     $('.source_link').on("click", function() { $('.list_sources').toggle(); $('#list_controls_menu').hide(); return false; });
     if (getListPathParam() == 'unread' && $('.menu_unread > a').css('font-weight') == 'bold') {
         $('.menu_unread > a').css('font-weight', 'normal');

diff --git a/modules/imap/functions.php b/modules/imap/functions.php
@@ -316,6 +316,7 @@ function format_imap_message_list($msg_list, $output_module, $parent_list=false,
                     array('safe_output_callback', 'source', $source),
                     array('safe_output_callback', 'from'.$nofrom, $from, null, str_replace(array($from, '<', '>'), '', $msg['from'])),
                     array('date_callback', $date, $timestamp),
+                    array('dates_holders_callback', $msg['internal_date'], $msg['date']),
                 ),
                 $id,
                 $style,
@@ -330,7 +331,8 @@ function format_imap_message_list($msg_list, $output_module, $parent_list=false,
                     array('safe_output_callback', 'from'.$nofrom, $from, null, str_replace(array($from, '<', '>'), '', $msg['from'])),
                     array('subject_callback', $subject, $url, $flags, null, $preview_msg),
                     array('date_callback', $date, $timestamp, $is_snoozed),
-                    array('icon_callback', $flags)
+                    array('icon_callback', $flags),
+                    array('dates_holders_callback', $msg['internal_date'], $msg['date']),
                 ),
                 $id,
                 $style,
@@ -1589,9 +1591,34 @@ function connect_to_imap_server($address, $name, $port, $user, $pass, $tls, $ima
     }
 }
 
-function getCombinedMessagesLists($sources, $cache, $searchTerms, $listPage, $limit, $offsets = [], $defaultOffset = 0, $filter = 'ALL') {
+/**
+ * @param array $sources
+ * @param object $cache
+ * @param array $search
+ */
+function getCombinedMessagesLists($sources, $cache, $search) {
+    $defaultSearch = [
+        'filter' => 'ALL',
+        'sort' => 'ARRIVAL',
+        'reverse' => true,
+        'terms' => [],
+        'limit' => 10,
+        'offsets' => [],
+        'defaultOffset' => 0,
+        'listPage' => 1
+    ];
+    $search = array_merge($defaultSearch, $search);
+
+    $filter = $search['filter'];
+    $sort = $search['sort'];
+    $reverse = $search['reverse'];
+    $searchTerms = $search['terms'];
+    $limit = $search['limit'];
+    $offsets = $search['offsets'];
+    $listPage = $search['listPage'];
+
     $totalMessages = 0;
-    $offset = $defaultOffset;
+    $offset = $search['defaultOffset'];
     $messagesLists = [];
     $status = [];
     foreach ($sources as $index => $dataSource) {
@@ -1604,14 +1631,27 @@ function getCombinedMessagesLists($sources, $cache, $searchTerms, $listPage, $li
 
         $mailbox = Hm_IMAP_List::get_connected_mailbox($dataSource['id'], $cache);
         if ($mailbox && $mailbox->authed()) {
+            $connection = $mailbox->get_connection();
+
             $folder = $dataSource['folder'];
-            $state = $mailbox->get_connection()->get_mailbox_status(hex2bin($folder));
+            $mailbox->select_folder(hex2bin($folder));
+            $state = $connection->get_mailbox_status(hex2bin($folder));
             $status['imap_'.$dataSource['id'].'_'.$folder] = $state;
 
-            $uids = $mailbox->search(hex2bin($folder), $filter, false, $searchTerms);
+            if ($mailbox->is_imap()) {
+                if ($connection->is_supported( 'SORT' )) {
+                    $sortedUids = $connection->get_message_sort_order($sort, $reverse, $filter);
+                } else {
+                    $sortedUids = $connection->sort_by_fetch($sort, $reverse, $filter);
+                }
+
+                $uids = $mailbox->search(hex2bin($folder), $filter, $sortedUids, $searchTerms);
+            } else {
+                // EWS
+                $uids = $connection->search($folder, $sort, $reverse, $filter, 0, $limit, $searchTerms);
+            }
+
             $total = count($uids);
-            // most recent messages at the top
-            $uids = array_reverse($uids);
             $uids = array_slice($uids, $offset, $limit);
 
             $headers = $mailbox->get_message_list(hex2bin($folder), $uids);
@@ -1662,3 +1702,15 @@ function flattenMessagesLists($messagesLists, $listSize) {
 
     return ['messages' => $endList, 'offsets' => $sizesTaken];
 }
+
+function sortCombinedMessages($list, $sort) {
+    usort($list, function($a, $b) use ($sort) {
+        $sortField = str_replace(['arrival', '-'], ['internal_date', ''], $sort);
+        if (strpos($sort, '-') === 0) {
+            return strtotime($a[$sortField]) - strtotime($b[$sortField]);
+        }
+        return strtotime($b[$sortField]) - strtotime($a[$sortField]);
+    });
+
+    return $list;
+}
diff --git a/modules/imap/handler_modules.php b/modules/imap/handler_modules.php
@@ -1261,6 +1261,8 @@ public function process() {
             $date = process_since_argument($this->user_config->get('all_since_setting', DEFAULT_SINCE));
         }
 
+        list($sort, $reverse) = process_sort_arg($this->request->get['sort'], $this->user_config->get('default_sort_order_setting', 'arrival'));
+
         $filter = 'ALL';
         $offset = 0;
         $list_page = 1;
@@ -1284,14 +1286,19 @@ public function process() {
             $offsets = explode(',', $offsets);
         }
 
-        $result = getCombinedMessagesLists($data_sources, $this->cache, [['SINCE', $date]], $list_page, $limit, $offsets, $offset, $filter);
+        $result = getCombinedMessagesLists($data_sources, $this->cache, [
+            'terms' => [['SINCE', $date]],
+            'listPage' => $list_page,
+            'limit' => $limit,
+            'offsets' => $offsets,
+            'defaultOffset' => $offset,
+            'filter' => $filter,
+            'sort' => $sort,
+            'reverse' => $reverse
+        ]);
 
         $list = flattenMessagesLists($result['lists'], $maxPerSource);
-        $messagesList = $list['messages'];
-
-        usort($messagesList, function($a, $b) {
-            return strtotime($b['internal_date']) - strtotime($a['internal_date']);
-        });
+        $messagesList = sortCombinedMessages($list['messages'], $this->request->get['sort']);
 
         $maxPages = ceil($result['total'] / $limit);
         $this->out('pages', $maxPages);
@@ -1328,6 +1335,7 @@ public function process() {
             return;
         }
 
+        list($sort, $reverse) = process_sort_arg($this->request->get['sort'], $this->user_config->get('default_sort_order_setting', 'arrival'));
         $list_page = (int) $this->request->get['list_page'];
         $offsets = $this->request->get['offsets'] ?? '';
         $keyword = $this->request->get['keyword'] ?? '';
@@ -1348,14 +1356,19 @@ public function process() {
         }
         $searchTerms[] = ['SINCE', $date];
 
-        $result = getCombinedMessagesLists($data_sources, $this->cache, $searchTerms, $list_page, $limit, $offsets, $offset, $filter);
+        $result = getCombinedMessagesLists($data_sources, $this->cache, [
+            'terms' => $searchTerms,
+            'listPage' => $list_page,
+            'limit' => $limit,
+            'offsets' => $offsets,
+            'defaultOffset' => $offset,
+            'filter' => $filter,
+            'sort' => $sort,
+            'reverse' => $reverse
+        ]);
 
         $list = flattenMessagesLists($result['lists'], $maxPerSource);
-        $messagesList = $list['messages'];
-
-        usort($messagesList, function($a, $b) {
-            return strtotime($b['internal_date']) - strtotime($a['internal_date']);
-        });
+        $messagesList = sortCombinedMessages($list['messages'], $this->request->get['sort']);
 
         $maxPages = ceil($result['total'] / $limit);
         $this->out('pages', $maxPages);
@@ -2091,6 +2104,7 @@ public function process() {
 
         $limit = $this->user_config->get($path.'_per_source_setting', DEFAULT_PER_SOURCE);
         $date = process_since_argument($this->user_config->get($path.'_since_setting', DEFAULT_SINCE));
+        list($sort, $reverse) = process_sort_arg($this->request->get['sort'], $this->user_config->get('default_sort_order_setting', 'arrival'));
 
         $maxPerSource = round($limit / count($data_sources));
         $offset = 0;
@@ -2109,14 +2123,19 @@ public function process() {
         }
         $searchTerms[] = ['SINCE', $date];
 
-        $result = getCombinedMessagesLists($data_sources, $this->cache, $searchTerms, $list_page, $limit, $offsets, $offset, 'ALL');
+        $result = getCombinedMessagesLists($data_sources, $this->cache, [
+            'listPage' => $list_page,
+            'limit' => $limit,
+            'offsets' => $offsets,
+            'defaultOffset' => $offset,
+            'terms' => $searchTerms,
+            'sort' => $sort,
+            'reverse' => $reverse,
+            'filter' => 'ALL'
+        ]);
 
         $list = flattenMessagesLists($result['lists'], $maxPerSource);
-        $messagesList = $list['messages'];
-
-        usort($messagesList, function($a, $b) {
-            return strtotime($b['internal_date']) - strtotime($a['internal_date']);
-        });
+        $messagesList = sortCombinedMessages($list['messages'], $this->request->get['sort']);
 
         $maxPages = ceil($result['total'] / $limit);
         $this->out('pages', $maxPages);

diff --git a/modules/imap/hm-imap.php b/modules/imap/hm-imap.php
@@ -2561,7 +2561,7 @@ protected function server_supports_custom_headers() {
             return true;
         }
 
-        protected function server_support_children_capability() {
+        public function server_support_children_capability() {
             $test_command = 'CAPABILITY'."\r\n";
             $this->send_command($test_command);
             $response = $this->get_response(false, true);

diff --git a/modules/imap/site.js b/modules/imap/site.js
@@ -465,11 +465,12 @@ var setup_imap_folder_page = async function(listPath, listPage = 1) {
     return [interval, backgroundAbortController];
 };
 
-$('#imap_filter_form').on('submit', async function(event) {
+$(document).on('submit', '#imap_filter_form', async function(event) { 
     event.preventDefault();
     const url = new URL(location.href);
     url.search = $(this).serialize();
     history.replaceState(null, '', url);
+    location.next = url.search;
     try {
         const messages = new Hm_MessagesStore(getListPathParam(), Hm_Utils.get_url_page_number());
         await messages.load(true);