Convert legacy color categories to new values throughout the code #2502

KaelenProctor · 2024-12-26T14:41:12Z

Fixes #2505

Problem

Cards in cubes may still have the legacy color category values such as "l", "w", or "m" rather than the current color category types of "Lands", "White", or "Multicolored" etc
These can be both on the card, and in the respective card details
Exporting the cube as CSV uses the legacy values if they are on the card
Importing did not convert legacy to current
The Color Category sort used the new values and so anything with the legacy values got stuck in the "Other" bucket, hence could import and see no cards as they were all unsorted
Cards with legacy color category could show in the card edit modal as having blank in the Color category dropdown. I think this occurred when the card details had legacy color category, but unsure. Sometimes the new value was selected, I think because the legacy values match the first letter (case insensitive) of the new values and browsers are flexible in that way
In a couple places the word "Multicolor" was used instead of "Multicolored", the latter which is in the color category type (and occurred more often in the codebase)

Solution

Updated the export and import process to explicitly covert from legacy to current color categories (if possible)
Updated the Color Category and Color Category full sorts to also convert
Converts from legacy to new whenever Card.cardColorCategory is called

Concerns

I am not sure whether the card dictionary JSONs should be converted to the new values for good
Should the placeholder card in carddb.getPlaceholderCard be updated?
Unsure why both the Card and Card details can container color category, and why one is colorCategory and the other colorcategory
Don't see any path to determine the color category as Hybrid from the card's color identity (in Card.cardColorCategory)

Testing

Existing behaviour

Used Cube's https://cubecobra.com/cube/list/jank and https://cubecobra.com/cube/list/trash-pile-test from the reported issues in Discord.

Exported both as CSV, and can see the legacy values of color category in them eg.

name,CMC,Type,Color,Set,Collector Number,Rarity,Color Category,status,Finish,maybeboard,image URL,image Back URL,tags,Notes,MTGO ID
"Grim Lavamancer",1,"Creature - Human Wizard",R,"dmr","324",rare,r,Owned,Foil,false,,,"","",107333
"Ignoble Hierarch",1,"Creature - Goblin Shaman",G,"mh2","414",rare,m,Owned,Foil,false,,,"","",91101

(r and m respectively)
2. Imported those into my local cube cobra using the current master branch. Many cards were categorized as "Other" using the color category sort (the default) per screenshots. For each first screenshot shows imported, then second shows after Display -> Show unsorted is on

Cube 1

Cube 2

Validated the legacy color categories in the cube JSON
Sorting by color identity works

New behaviour

The color category sorting works for the cubes imported from the previous section (no edits or anything to the cards)
Importing the cubes from CSV correctly has color category set
New color categories are saved in the cube JSON (on the card itself)
CSV export uses the current color categories (r -> Red in comparison)

"Grim Lavamancer",1,"Creature - Human Wizard",R,"dmr","324",rare,Red,Owned,Foil,false,,,"","",107333

Ran the import cards script and validated the color category values are the new ones, not the old ones

in cubes which had those values explicitly saved.

KaelenProctor · 2024-12-26T14:44:41Z

src/datatypes/CardDetails.ts

-  | 'Hybrid'
-  | 'Lands';
+
+export const COLOR_CATEGORIES = ['White', 'Blue', 'Black', 'Red', 'Green', 'Colorless', 'Multicolored', 'Hybrid', 'Lands'] as const;


Using typescript const assertions so that we can use COLOR_CATEGORIES to check if a string is within its set of values. Could not see how to do that using the ColorCategory type itself.

KaelenProctor · 2024-12-26T14:44:56Z

serverjs/cubefn.js

@@ -228,6 +229,9 @@ function CSVtoCards(csvString, carddb) {
  } of camelizedRows) {
    if (name) {
      const upperSet = (set || '').toUpperCase();
+
+      const validatedColorCategory = convertFromLegacyCardColorCategory(colorCategory);


This applies when importing the CSV

KaelenProctor · 2024-12-26T14:45:04Z

routes/cube/helper.js

@@ -195,14 +196,17 @@ function writeCard(res, card, maybe) {
  } else {
    imgBackUrl = '';
  }
+
+  const colorCategory = cardutil.convertFromLegacyCardColorCategory(card.colorCategory);


Applies when writing the CSV

KaelenProctor · 2024-12-26T14:46:51Z

src/utils/Card.ts

@@ -203,7 +236,7 @@ export const cardColorCategory = (card: Card): ColorCategory => {
    return 'Colorless';
  }
  if (colors.length > 1) {
-    return 'Multicolor';
+    return 'Multicolored';


Found when exploring why some cards didn't show the right value in the Color Category dropdown, looked to be a recent typo. More places used Multicolored including on the Color category type

lunakv · 2024-12-27T18:27:16Z

Ad concern 3, I'm pretty sure the idea is that "details" contains immutable information (from the user's perspective) directly loaded from the carddb JSONs, so it contains the default color category that is assigned to cards with this cardID. If a user overrides that default in their cube, it changes the color category directly on the Card object, which takes precedence over the default in "details".

Since it looks like every Card has its colorCategory set explicitly to the default when created, you're right that it could be omitted from the details, but since those are just a copy-pasted object, there's not much of a point in explicitly deleting a single value.

Ad concern 1, I'd say that depends on if the value is used anywhere (other than as a default for Card objects). If not, I think you can just change them and get rid of an inconsistency. If yes, then it depends on how much work changing all the usages is. One place I'd check for sure is card search.

Ad concern 2, I think getPlaceholderCard should reflect whatever the format of actual cards in the carddb ends up being.

Ad concern 4, who says you have to determine it based on color identity? The function gets the entire Card as input, so assuming it has details set (which I'm admittedly unsure of), you can just use CARD_CATEGORY_DETECTORS.hybrid.

A word of caution here, though. The current behavior (buggy or not) means Hybrid is a category that has to be set explicitly, and it is never assigned to cards by default. So if you change that, you are possibly creating a situation where existing hybrid cards are sorted under Multicolored, but newly added hybrid cards appear under Hybrid, which feels like undesirable behavior with wide-reaching impact. Color Category is the default primary sort for all cubes, so any changes to its behavior should ve really carefully considered.

Colorless value.

has to be set by the cube owners.

KaelenProctor · 2024-12-28T14:27:08Z

Thanks for the feedback and additional details about my concerns!

Ad concern 1, I'd say that depends on if the value is used anywhere (other than as a default for Card objects). If not, I think you can just change them and get rid of an inconsistency. If yes, then it depends on how much work changing all the usages is. One place I'd check for sure is card search.

In my testing the conversion of the legacy to current color categories don't update on cube JSON unless the color category explicitly changes. eg if a card in the cube JSON has legacy color category "b" instead of "Black" (which the UI will convert that to "Black" for the sorting/display) and then another field on the card changes such as status, the save will not touch the color category field.

The next time the cards list is updated (via jobs/update_cards.js) the carddict.json will use the new colorcategory values. Then afterwards cards added to a cube will use the current color category saved within the card dictionary. And if the card dictionary isn't updated, the legacy to current color category conversions will continue to apply at runtime when the cube UI shows.

Ad concern 2, I think getPlaceholderCard should reflect whatever the format of actual cards in the carddb ends up being.

I've updated the color category to Colorless. I tested by manually mucking one of the card ids in a cube JSON in my local s3, such that the card could not be found and carddb.getPlaceholderCard had to execute. I was able to change the color category via the modal and validate that saved on the card, whereas the card details stayed as Colorless.

Unsure why both the Card and Card details can container color category, and why one is colorCategory and the other colorcategory

Thanks for explaining. I did further testing and validated that updating the color category on a card only changes the colorCategory on the card, not within the card details.

Ad concern 4, who says you have to determine it based on color identity? The function gets the entire Card as input, so assuming it has details set (which I'm admittedly unsure of), you can just use CARD_CATEGORY_DETECTORS.hybrid.

A word of caution here, though. The current behavior (buggy or not) means Hybrid is a category that has to be set explicitly, and it is never assigned to cards by default. So if you change that, you are possibly creating a situation where existing hybrid cards are sorted under Multicolored, but newly added hybrid cards appear under Hybrid, which feels like undesirable behavior with wide-reaching impact. Color Category is the default primary sort for all cubes, so any changes to its behavior should ve really carefully considered.

Ah, that is helpful to understand. I assumed that there would be some way to detect hybrid for the color category, as you say given the totality of the card details. The current code only uses color identity but as you say it doesn't have to be limited to that, given the casting cost contains the hybrid symbols. Knowing that hybrid is only used when explicitly set makes things simple, meaning there is no additional changes necessary.

KaelenProctor added 7 commits December 25, 2024 10:01

Convert legacy color category values to current, to handle cards

db491cd

in cubes which had those values explicitly saved.

Ensure all Multicolored references are equivalent.

0463066

Ensure Color Categories on export are current values, not legacy

42856f5

Convert legacy color category to current during the import CSV process.

6d62db8

Reduce duplication by using constant for the Color Category sort values.

d677df1

Convert the color category from legacy to current in the Sort process.

d13dfa3

Use current color category values in the card update job.

0a67ca2

KaelenProctor commented Dec 26, 2024

View reviewed changes

KaelenProctor marked this pull request as ready for review December 26, 2024 14:47

KaelenProctor added 2 commits December 28, 2024 08:18

Update the placeholder card's Color category to match the current type's

b3b13ca

Colorless value.

Add note about Hybrid color category is not auto-detected,

eab9f66

has to be set by the cube owners.

dekkerglen merged commit 3a8f113 into dekkerglen:master Dec 30, 2024
1 check failed

KaelenProctor deleted the feature/legacy-color-category-conversions branch December 30, 2024 19:17

KaelenProctor mentioned this pull request Dec 30, 2024

Replace with CSV File Upload Not Working #2505

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert legacy color categories to new values throughout the code #2502

Convert legacy color categories to new values throughout the code #2502

KaelenProctor commented Dec 26, 2024 •

edited

Loading

KaelenProctor Dec 26, 2024

KaelenProctor Dec 26, 2024

KaelenProctor Dec 26, 2024

KaelenProctor Dec 26, 2024

lunakv commented Dec 27, 2024

KaelenProctor commented Dec 28, 2024 •

edited

Loading

Convert legacy color categories to new values throughout the code #2502

Convert legacy color categories to new values throughout the code #2502

Conversation

KaelenProctor commented Dec 26, 2024 • edited Loading

Problem

Solution

Concerns

Testing

Existing behaviour

New behaviour

KaelenProctor Dec 26, 2024

Choose a reason for hiding this comment

KaelenProctor Dec 26, 2024

Choose a reason for hiding this comment

KaelenProctor Dec 26, 2024

Choose a reason for hiding this comment

KaelenProctor Dec 26, 2024

Choose a reason for hiding this comment

lunakv commented Dec 27, 2024

KaelenProctor commented Dec 28, 2024 • edited Loading

KaelenProctor commented Dec 26, 2024 •

edited

Loading

KaelenProctor commented Dec 28, 2024 •

edited

Loading