Merge pull request #2601 from KaelenProctor/feature/normalized-sortin…

…g-of-collection-numbers

Improve sorting of collection numbers to be more numeric like, as Human's would expect

src/client/utils/Sort.ts

@@ -214,6 +214,43 @@ export const ORDERED_SORTS: string[] = [
   'Collector number',
 ];
 
+/**
+ * Pad the collector number with zeros in order for string sorting to work numerically like.
+ * eg After padding, 100 will be greater than 20 in a string comparison
+ */
+const normalizeCollectorNumberForSorting = (card: Card): string => {
+  let collectorNumber = cardCollectorNumber(card);
+  const PAD_AMOUNT = 10;
+
+  /* If the collector number ends with a non-digit, then take it off while padding and append after.
+   * eg. 100★, 55†, 77c
+   * The goal here is to pad the numeric portion of the collector number such that they sort, irrespective of the ending
+   * character. eg 100★ vs 55† => 00100 vs 00055 (padding to 5 for conciseness) instead of 0100★ vs 0055†.
+   * This isn't by any means perfect, as it doesn't work well for cards like Unknown Event: UA03a
+   */
+  let lastCharacter = collectorNumber[collectorNumber.length - 1];
+  if (isNaN(parseInt(lastCharacter, 10))) {
+    collectorNumber = collectorNumber.slice(0, collectorNumber.length - 1);
+  } else {
+    lastCharacter = '';
+  }
+
+  /* The List or Championship promo cards have a dash in their number.
+   * For List cards its in form of SET-NUMBER, and promos are YEAR-NUMBER.
+   * So for these to sort within their groups, we want to pad both sides
+   */
+  const sides = collectorNumber.split('-');
+  if (sides.length === 2) {
+    const [left, right] = sides;
+    //eg. After padding, SHM-120 should come after SHM-44
+    collectorNumber = left.padStart(PAD_AMOUNT, '0') + '-' + right.padStart(PAD_AMOUNT, '0');
+  } else {
+    collectorNumber = collectorNumber.padStart(PAD_AMOUNT, '0');
+  }
+
+  return collectorNumber + lastCharacter;
+};
+
 export const SortFunctions: Record<string, (a: any, b: any) => number> = {
   Alphabetical: alphaCompare,
   'Mana Value': (a, b) => cardCmc(a) - cardCmc(b),
@@ -231,10 +268,12 @@ export const SortFunctions: Record<string, (a: any, b: any) => number> = {
   'Cube Count': (a, b) => cardCubeCount(a) - cardCubeCount(b),
   'Pick Count': (a, b) => cardPickCount(a) - cardPickCount(b),
   'Collector number': (a, b) => {
-    if (cardCollectorNumber(a) > cardCollectorNumber(b)) {
+    const aCollectorNumber = normalizeCollectorNumberForSorting(a);
+    const bCollectorNumber = normalizeCollectorNumberForSorting(b);
+    if (aCollectorNumber > bCollectorNumber) {
       return 1;
     }
-    if (cardCollectorNumber(a) < cardCollectorNumber(b)) {
+    if (aCollectorNumber < bCollectorNumber) {
       return -1;
     }
     return 0;

tests/cards/sort.test.ts

@@ -0,0 +1,185 @@
+import { sortForDownload } from '../../src/client/utils/Sort';
+import Card from '../../src/datatypes/Card';
+import { createCard, createCardDetails } from '../test-utils/data';
+
+describe('Sorting', () => {
+  const COLLECTOR_NUMBER_SORT = 'Collector number';
+
+  const sortWithoutGrouping = (cards: Card[], sort: string) => {
+    //Must pass true for showOther because the first sort is undefined
+    return sortForDownload(cards, undefined, undefined, undefined, sort, true);
+  };
+
+  const mapToCardNames = (sorted: Card[]) => {
+    return sorted.map((c) => c.details?.name);
+  };
+
+  /* This is not an exhaustive list of non-standard collector numbers, but various examples to illustrate
+   * how collector numbers are not simple numbers.
+   */
+  const EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS = {
+    PLANESWALKER_CHAMPIONSHIP_PROMO_CN: '2024-03', //eg. https://scryfall.com/search?as=grid&order=name&q=%28game%3Apaper%29+set%3Apwcs
+    SET_PROMOS: '49p', //eg. https://scryfall.com/search?as=grid&order=name&q=%28game%3Apaper%29+set%3Apblb
+    MAGIC_ONLINE_THEME_DECKS: 'B14', //eg. https://scryfall.com/sets/td0?as=grid&order=set
+    UNKNOWN_EVENT: 'UA03a', //eg. https://scryfall.com/search?as=grid&order=name&q=%28game%3Apaper%29+set%3Ada1
+    CARD_ART_VARIANTS: '10a', //eg. https://scryfall.com/card/fem/10a/icatian-moneychanger
+    WORLD_CHAMPIONSHIP_DECKS: 'ap100', //eg. https://scryfall.com/search?as=grid&order=name&q=%28game%3Apaper%29+set%3Awc04
+    PROMO_CARD_VARIANT: '75★', //eg. https://scryfall.com/card/pths/75%E2%98%85/abhorrent-overlord
+    STARTER_CARD_VARIANT: '101†', //eg. https://scryfall.com/card/aer/101%E2%80%A0/wrangle
+    THE_LIST: 'SHM-150', //eg. https://scryfall.com/search?as=grid&order=name&q=%28game%3Apaper%29+set%3Aplst
+    MAGIC_ONLINE_PROMO: '95299', //eg. https://scryfall.com/search?order=set&q=set%3Aprm+year%3A2025&unique=prints
+  };
+
+  it('Sorting by collector number should be numeric-like', async () => {
+    const cards = [
+      createCard({
+        details: createCardDetails({ name: 'Card 1', collector_number: '10' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 2', collector_number: '5' }),
+      }),
+    ];
+
+    const sorted = sortWithoutGrouping(cards, COLLECTOR_NUMBER_SORT);
+
+    expect(mapToCardNames(sorted)).toEqual(['Card 2', 'Card 1']);
+  });
+
+  /* List cards collector numbers are in the form of SET-NUMBER, eg C14-145 or SHM-150
+   */
+  it('Sorting by collector number should be numeric-like, for list cards in the same set', async () => {
+    const cards = [
+      createCard({
+        details: createCardDetails({ name: 'Card 1', collector_number: 'SHM-47' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 2', collector_number: 'SHM-150' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 3', collector_number: 'SHM-5' }),
+      }),
+    ];
+
+    const sorted = sortWithoutGrouping(cards, COLLECTOR_NUMBER_SORT);
+
+    expect(mapToCardNames(sorted)).toEqual(['Card 3', 'Card 1', 'Card 2']);
+  });
+
+  it('Sorting by collector number with card variants', async () => {
+    const cards = [
+      createCard({
+        details: createCardDetails({ name: 'Card 1', collector_number: '75' }),
+      }),
+      createCard({
+        details: createCardDetails({
+          name: 'Card 2',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.STARTER_CARD_VARIANT,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({
+          name: 'Card 3',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.PROMO_CARD_VARIANT,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 4', collector_number: '101' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 5', collector_number: '101★' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 6', collector_number: '75b' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 7', collector_number: '75a' }),
+      }),
+    ];
+
+    const sorted = sortWithoutGrouping(cards, COLLECTOR_NUMBER_SORT);
+
+    expect(mapToCardNames(sorted)).toEqual(['Card 1', 'Card 7', 'Card 6', 'Card 3', 'Card 4', 'Card 2', 'Card 5']);
+  });
+
+  it('Sorting by collector number for List cards (dash separated to be exact)', async () => {
+    const cards = [
+      createCard({
+        details: createCardDetails({
+          name: 'Card 1',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.THE_LIST,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 2', collector_number: 'SHM-20' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 3', collector_number: 'SHM-188' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 4', collector_number: 'ORI-38' }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 5', collector_number: 'TMP-304' }),
+      }),
+      //Will be last because 2024 has more characters than any 3 letter set symbol
+      createCard({
+        details: createCardDetails({
+          name: 'Card 6',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.PLANESWALKER_CHAMPIONSHIP_PROMO_CN,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({ name: 'Card 7', collector_number: '8ED-72' }),
+      }),
+    ];
+
+    const sorted = sortWithoutGrouping(cards, COLLECTOR_NUMBER_SORT);
+
+    expect(mapToCardNames(sorted)).toEqual(['Card 7', 'Card 4', 'Card 2', 'Card 1', 'Card 3', 'Card 5', 'Card 6']);
+  });
+
+  it('Sorting by collector number with many non-standard numbers', async () => {
+    const cards = [
+      createCard({
+        details: createCardDetails({
+          name: 'Card 1',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.MAGIC_ONLINE_PROMO,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({
+          name: 'Card 2',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.WORLD_CHAMPIONSHIP_DECKS,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({
+          name: 'Card 3',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.UNKNOWN_EVENT,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({
+          name: 'Card 4',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.MAGIC_ONLINE_THEME_DECKS,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({
+          name: 'Card 5',
+          collector_number: EXAMPLE_NON_STANDARD_COLLECTOR_NUMBERS.PLANESWALKER_CHAMPIONSHIP_PROMO_CN,
+        }),
+      }),
+      createCard({
+        details: createCardDetails({
+          name: 'Card 6',
+          collector_number: '42',
+        }),
+      }),
+    ];
+
+    const sorted = sortWithoutGrouping(cards, COLLECTOR_NUMBER_SORT);
+
+    expect(mapToCardNames(sorted)).toEqual(['Card 6', 'Card 4', 'Card 5', 'Card 3', 'Card 1', 'Card 2']);
+  });
+});