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

feat(ModuleInstaller): Add method to rename tables in the DB #623

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

Hipska
Copy link
Contributor

@Hipska Hipska commented Feb 28, 2024

Base information

Question Answer
Related to a SourceForge thead / Another PR / Combodo ticket? N/A
Type of change? Enhancement

Objective

To have a helper method that can rename a table in DB during setup. The helper method can silently ignore the renaming if the origin table doesn't exist or the destination table already exists. This to avoid errors when running the setup again after the migration has ben done before.

Proposed solution

Creation of ModuleInstallerAPI::MoveColumnInDB.

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have tested all changes I made on an iTop instance
  • I have added a unit test, otherwise I have explained why I couldn't
  • Is the PR clear and detailled enough so anyone can understand digging in the code?

@Molkobain Molkobain added enhancement New feature or request core labels Feb 28, 2024
Copy link
Member

@Molkobain Molkobain left a comment

Choose a reason for hiding this comment

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

See comment on test

@Molkobain Molkobain self-assigned this Feb 28, 2024
Copy link
Contributor

@piRGoif piRGoif left a comment

Choose a reason for hiding this comment

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

Great work, thanks !
My remarks are mostly questions, and also we may add more uses cases in the phpunit

setup/moduleinstaller.class.inc.php Outdated Show resolved Hide resolved
setup/moduleinstaller.class.inc.php Show resolved Hide resolved
setup/moduleinstaller.class.inc.php Show resolved Hide resolved
setup/moduleinstaller.class.inc.php Show resolved Hide resolved
setup/moduleinstaller.class.inc.php Show resolved Hide resolved
* @throws \CoreException
* @throws \MySQLException
*/
public function testRenameTableInDB()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also test :

  • orig and dest on the same existing table name
  • orig and dest on the same non existing table name
  • orig on a non existing table name
  • dest on an existing table name

This may be easier to code using a dataprovider returning orig and dest names, and expected result ?

Hipska and others added 2 commits March 2, 2024 12:14
Co-authored-by: Pierre Goiffon <pierre.goiffon@combodo.com>
Copy link
Member

@Molkobain Molkobain left a comment

Choose a reason for hiding this comment

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

Technical review:

  • Unit test:
    • Move table revert in tearDown() method for better robustness in case there is an issue within the test method
  • New method:
    • Add SetupLog message in exit conditions (except the !MetaModel::DBExists(false)). See RenameEnumValueInDB() for a real example, MoveColumnInDB() is a bad one.

# Conflicts:
#	tests/php-unit-tests/unitary-tests/setup/ModuleInstallerAPITest.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
Status: Pending technical review
3 participants