twentyhq / twenty

Building a modern alternative to Salesforce, powered by the community.
https://twenty.com
GNU Affero General Public License v3.0
15.84k stars 1.74k forks source link

Sorting by name doesn't show all the contact #7095

Open Sytten opened 2 days ago

Sytten commented 2 days ago

Bug Description

If you sort by name there is a bug where not all the contacts are shown. If you see the screenshot, I sorted by name and selected all the rows manually. It shows that there are 123 rows selected out of my 515 contacts.

Screenshot 2024-09-17 at 8 22 28 AM

Expected behavior

All rows should be shown when sorting.

Technical inputs

The cursor pagination of the table seems ok on the frontend side as far as I can tell. The paging is done in batches of 59 contacts (that is weird). But cursor pagination is very tricky to implement server side especially when doing it on multiple field. I have done an implementation of if myself in rust (not oss) and went through all the possible problems you might have. Here is a snippet of how I went about it:

// The logic is pretty complex to make that work for all cases.
/// Keep that in mind when changing code, the various clauses were
/// added for a reason. Please read carefully:
///
///   - The `before` and `after` cursors are basically mirrors of each other.
///     This allows us to share most of the code.
///
///   - The general logic of cursor pagination with ordering is that you first filter and order
///     by the cursor value and then by ID. This is because the cursor value probably isn't unique
///     so we need to use a unique column for "total" ordering within a given cursor value.
///     The end goal is to respect the cursor value ordering, but ALWAYS have the ID as ascending.
///     For example, with status code: (200, 2); (200, 3); (404, 1).
///
///   - When using a `last`, we need to reverse so we can use limit in SQL.
///     Essentially think of limit as `first`, so we need the logic need to be "ascending".
///     This means basically looking at an ordering the other way around. This applies to
///     both the ordering of the cursor value AND the ordering of the ID.
///     For example, with status code: (404, 1); (200, 3); (200, 2).
///     We then reverse the result set in software after the fetch.
///
///   - The value `null` is a PITA. In sqlite both `null > 1` and `null < 1` return `null`,
///     so it is basically unusable. Since `null` is the lowest value, for greater-than we
///     use `IS NOT NULL`. We currently don't have negative values, but even if we did we would want
///     them after `NULL`. For less-than, it is a bit more complicated since we do need to include the
///     null with an `IS NULL` clause. But if the cursor value itself is null, we don't include that
///     clause otherwise it bypasses the ID filtering.
///

pub fn apply_after_cursor<T>(
    query: &mut SelectStatement,
    after: &proxy_domain::Cursor,
    order: &Option<SeaOrder>,
) where
    T: Identifiable,
{
    match order {
        // CASE: last + desc order
        Some(order) if order.ordering == Order::Asc && order.reversed => {
            apply_lt_value_gt_id::<T>(query, after, order);
        }
        // CASE: last + asc order
        Some(order) if order.ordering == Order::Desc && order.reversed => {
            apply_gt_value_gt_id::<T>(query, after, order);
        }
        // CASE: first/all + asc order
        Some(order) if order.ordering == Order::Asc => {
            apply_gt_value_gt_id::<T>(query, after, order);
        }
        // CASE: first/all + desc order
        Some(order) if order.ordering == Order::Desc => {
            apply_lt_value_gt_id::<T>(query, after, order);
        }
        _ => {
            query.cond_where(Expr::col(T::get_id_column()).gt(after.id));
        }
    }
}
Bonapara commented 2 days ago

@Weiko, can you take a look? We couldn't reproduce the bug with @charlesBochet. However, during testing, we encountered a related issue. When trying to export a list of 1,200 people sorted by name, only the first page was exported.

Sytten commented 1 day ago

If you need to test in prod using my account just let me know, it's easy to reproduce. It doesn't seem like it is just the first page since I see names going to T. Might be a bug with missing last name? Since it sorts as first name and last name?

Bonapara commented 1 day ago

Can you reproduce it on demo.twenty.com? Auth: noah@demo.dev and Applecar2025

Tried again but couldn't reproduce the issue, even with a missing last name 🤔