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

[Hipska]

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.
EDIT: This part has been rejected by Combodo technical review..

Proposed solution

Creation of ModuleInstallerAPI::RenameTableInDB.

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]

See comment on test

[piRGoif]

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

[piRGoif]

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 ?

[Molkobain]

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.

[Hipska]

Okay @Molkobain, but earlier you said this:

So it's best to revert the table to its original state in order to keep it clean for other tests. Moving the revert in the teardown method would not pollute other test classes, but it would impact other tests from the current class.

So I understand that it might impact other tests and thus it needed to be added within the test?

About the exit conditions, I thought this was also discussed and agreed before?

Ok right, indeed there are only return in \ModuleInstallerAPI::MoveColumnInDB This method was added in 2018 so not to far away, it is quite a strange choice :/ To be discussed during next review !

Followed by

Closing conversation, we discussed during tech review and this is no longer valid.

Do I understand this wrong?

[Hipska]

@Molkobain?

[Molkobain]

Hello Thomas, nevermidn about the revert table comment, leave it as it is.
Just missing the SetupLog messages, then alright for me. :)

[Molkobain]

Thanks!