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

fix: v13 -> v14 migration issues #448

Merged
merged 7 commits into from
Apr 17, 2023

Conversation

ruchamahabal
Copy link
Member

@ruchamahabal ruchamahabal commented Apr 17, 2023

Fixtures

Ignore mandatory fields while installing default salary components on app installation. Migration should not break if standard fields are customized as mandatory.

KRA patching

Problem:

  • In feat: Performance Module #43 KRA field in Appraisal Template was changed from Small Text to Link. Patch for this works fine for sites that are already on v14
  • But while migrating from v13 using a site backup, if there is existing KRA data > 140 chars, installation fails because patches cannot run before HR app installation.

Solution:

To ensure backward compatibility during installation:

  • Instead of changing datatype of the original KRA column (Small Text), add a new Link field Key Result Area
  • Copy KRA (Small Text)'s trimmed content to new key_result_area Link field
  • Refactor KRA field's references: kra -> key_result_area
  • Move performance module patch to post install patches since patching before schema update is not required anymore

Post Install Patches

  • Patches updating some data should also run post installation to avoid leaving data in an inconsistent state during v13 -> v14 migration
  • Run required patches post installation too
  • Problem: If the site is created from a backup, all the patches are set as patched in patch log, so even if the patch has not run on the site, the entry will be present in patch log. Run all post-install patches without checking patch log.
  • refactor: fail-safe post install patches
    • add ignore mandatory flags wherever applicable
    • clear dt cache after altering table directly
    • convert subquery updates to join query updates in QB
    • make sure data doesn't get overwritten if patch runs more than once
    • remove unnecessary post install patches

@codecov-commenter
Copy link

codecov-commenter commented Apr 17, 2023

Codecov Report

Merging #448 (c0cf1e2) into develop (77d51ab) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #448      +/-   ##
===========================================
- Coverage    72.89%   72.88%   -0.02%     
===========================================
  Files          190      190              
  Lines         9789     9790       +1     
===========================================
- Hits          7136     7135       -1     
- Misses        2653     2655       +2     
Impacted Files Coverage Δ
hrms/hr/doctype/appraisal/appraisal.py 81.14% <ø> (ø)
hrms/overrides/company.py 71.01% <100.00%> (+0.42%) ⬆️

... and 1 file with indirect coverage changes

- while migrating from v13 if there is existing data > 140 chars in KRA, installation fails because patches cannot run before installation

- add a new Link field key result area

- copy KRA (Small Text)'s trimmed content to new key_result_area Link field
- since it does not need access to old schema now that key_result_area is added as a new field instead of changing the existing one
- patches updating some data should also run post install to avoid leaving data in an inconsistent state during v13 -> v14 migration
- if site is created from a backup, all the patches are set as patched in patch log

- so even if the patch has not run on the site, the entry will be present in patch log
- add ignore mandatory flags wherever applicable

- clear dt cache after altering table directly

- convert subquery updates to join query updates in QB

- make sure data doesn't get overwritten if patch runs more than once

- remove unnecessary post install patches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants