LPS-88901 Ensure AssetEntryAssetCategoryRel table is created before running the upgrade, similar to how layout-page-template-service creates its table prior to upgrading"

[ealonso]

No description provided.

[liferay-continuous-integration]

To conserve resources, the PR Tester does not automatically run for every pull.

If your code changes were already tested in another pull, reference that pull in this pull so the test results can be analyzed.

If your pull was never tested, comment "ci:test" to run the PR Tester for this pull.

[achaparro]

Just started reviewing :)

Sent from GH.

[achaparro]

Pull request submitted to brianchandotcom#66550.

Sent from GH.

[achaparro]

Hi @SamZiemer,

I really liked your changes because they solve the issue in the simplest way and they follow the good practices for upgrades 👍

Once Brian approves your changes, I would have some questions regarding this:

• Should we revert the changes implemented here https://issues.liferay.com/browse/LPS-85497 (at least in master) since they add backward compatibily when it's not longer needed?
• This question is for @ealonso, do we still use the table AssetEntries_AssetCategories after creating and populating AssetEntryAssetCategoryRel?

Thanks.

[ealonso]

@achaparro we shouldn't use this table anymore.

[SamZiemer]

Hi @achaparro,

Thank you for reviewing this fix. I am not sure how LPS-85497 relates to this fix though.

[achaparro]

Sorry @SamZiemer! I meant this one: https://issues.liferay.com/browse/LPS-85783

[SamZiemer]

Oh, yes, I was wondering the same question about LPS-85783. I had meant to ask in my initial pull request of that should be removed with this fix implemented, but I seem to have left it off.

From what I can tell, it looks like we could safely remove that ticket, however, I wanted to get the opinion of yourself and @ealonso on the matter before we do revert it.

[achaparro]

Hi @SamZiemer,

I have been talking to @ealonso and he also agrees to remove or depreate the code added by LPS-85783 in master, I have created the following ticket for it https://issues.liferay.com/browse/LPS-89064.

Since you have analyzed both approaches and know what we should remove, could you take care of it?

Apart from this, I have created a new ticket to remove the old table and its related code in the future:
https://issues.liferay.com/browse/LPS-89065

Thanks!

[ealonso]

Hey @achaparro @SamZiemer,

We cannot deprecated the code added by LPS-85783, since it is adding a compatibility layer with Services, not with the DB, this is necessary because the users still using the old service, since we don't have those services extracted into modules.

Sorry for the confusion.

Regards,
Eudaldo.

[achaparro]

Ok @ealonso, should we remove (only in master) at least the following code?:
https://github.com/liferay/liferay-portal/blob/master/modules/apps/asset/asset-categories-service/src/main/java/com/liferay/asset/categories/internal/service/AssetEntryAssetCategoryRelAssetCategoryLocalServiceWrapper.java#L251-L261

which gets the categories from the new table and also for the old table? That is not longer needed after the changes made by https://issues.liferay.com/browse/LPS-88901

Thanks.

cc/ @SamZiemer

[ealonso]

@achaparro, Yes we should remove that code, as you said this is no longer needed after https://issues.liferay.com/browse/LPS-88901

[achaparro]

Hi @ealonso,

I have sent a pull request to remove that code:
ealonso#2071

Thanks!

PS: @SamZiemer I have taken care of this issue since it's trivial now. Do you agree with our final conclusion?

[SamZiemer]

Hi @achaparro, Yes I think the solution makes sense, thank you for doing it.