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

[#1405] Migrate existing class saves/skills data to TraitAdvancement #2546

Merged
merged 7 commits into from
Nov 13, 2023

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Nov 1, 2023

Migrates existing entries from the saves and skills sections on class items into TraitAdvancement.

@arbron arbron added this to the D&D5E 2.4.0 milestone Nov 1, 2023
@arbron arbron self-assigned this Nov 1, 2023
@arbron arbron requested a review from Fyorl November 1, 2023 20:31
@arbron arbron mentioned this pull request Nov 1, 2023
7 tasks
@arbron arbron force-pushed the advancement/trait-migration branch from cef72cd to acd500a Compare November 1, 2023 21:03
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some additional considerations:

  1. We should consider doing the trait migration as a full migration to ensure the data gets persisted. In-memory migrations are fine for changing names or types, but since we're writing full advancement objects I'd feel more comfortable doing it as a full migration.
  2. I think we can also strip out some code related to configuring skills and saves from ItemSheet5e, and potentially also trim some functionality from TraitSelector, though I'm less sure about that.

module/data/item/class.mjs Show resolved Hide resolved
@arbron arbron requested a review from Fyorl November 7, 2023 22:16
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes, Jeff. I think this is a good opportunity to build up some framework for persisting source migrations that we can make more use of in the future.

Two things I want to make absolutely sure of when testing this:

  • If the migration is run a second time, items do not repeat the 'persist source migration' workflow with diff: false.
  • After the migration is performed, no items are left with dnd5e.needsMigration flags.

module/data/item/class.mjs Outdated Show resolved Hide resolved
module/data/item/class.mjs Outdated Show resolved Hide resolved
module/migration.mjs Outdated Show resolved Hide resolved
module/migration.mjs Outdated Show resolved Hide resolved
@arbron arbron requested a review from Fyorl November 8, 2023 16:27
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, Jeff, just a couple of things flagged. I have one issue raised in my testing that I hope you can reproduce:

  1. (on 2.4.x branch) Create a new world
  2. Create a new actor
  3. Create a new class on that actor
  4. Edit the class and give it any advancement (I used HitPointAdvancement)
  5. Under its details tab, configure 2 saving throws and some skills (I used 6 skills)
  6. Select 2 skills (also under the details tab)
  7. Close down the world and switch to the trait-migration branch
  8. Launch the world and run migrateWorld
  9. Open the class item on the actor and select 'modify choices'
  10. Advance to the skill trait flow
  11. Observe no saved choices or choices to make

image

Also noticed a small display issue with the alignment of the icons in the advancement table. If you happen to know what the issue is there, feel free to make a quick fix directly. Otherwise I will take a look in due course.

image

module/migration.mjs Outdated Show resolved Hide resolved
module/migration.mjs Outdated Show resolved Hide resolved
module/migration.mjs Outdated Show resolved Hide resolved
@arbron
Copy link
Collaborator Author

arbron commented Nov 13, 2023

Looking good, Jeff, just a couple of things flagged. I have one issue raised in my testing that I hope you can reproduce:

That is because this branch was created before TraitFlow was merged and never rebased, so it just displays the summary but doesn't actually apply anything.

Also noticed a small display issue with the alignment of the icons in the advancement table. If you happen to know what the issue is there, feel free to make a quick fix directly. Otherwise I will take a look in due course.

Not sure what you're referring to. Is it because the icons are aligned at the top with the title?

@arbron arbron requested a review from Fyorl November 13, 2023 21:38
@Fyorl
Copy link
Contributor

Fyorl commented Nov 13, 2023

Not sure what you're referring to. Is it because the icons are aligned at the top with the title?

Just that they seem completely flush as to be touching the border of the element above them. I think it might just be a property of these particular icons though. I think others seem to have a bit of space baked in at the top so that I didn't notice it before.

Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was able to do some further testing after the rebase, and it looks pretty good to me, thanks

@arbron arbron merged commit 39e9416 into foundryvtt:2.4.x Nov 13, 2023
@arbron arbron deleted the advancement/trait-migration branch November 13, 2023 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants