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.

Sign up for GitHub

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

Jump to bottom

[TS] LPS-134264 #4885

Closed
wants to merge 1 commit into from
Closed

[TS] LPS-134264 #4885

wants to merge 1 commit into from

Conversation

jonathanmccann
Copy link

@jonathanmccann jonathanmccann commented Jun 15, 2021 •
edited

Prior to https://issues.liferay.com/browse/LPS-128817 groups were filtered using the following logic:

https://github.com/liferay/liferay-portal-ee/blob/fix-pack-dxp-12-7210/modules/apps/site/site-browser-web/src/main/java/com/liferay/site/browser/web/internal/display/context/SiteBrowserDisplayContext.java#L398-L404

After LPS-128817 filtering only took place if:

  1. filterManageableGroups is true
  2. A value for usersGroups is supplied to the group search

This change in behavior allowed all sites to be returned when a user was adding another user to a site (https://issues.liferay.com/browse/LPS-134264).

The changes in this pull attempt to bring back the previous behavior by checking for the "Assign Members" permission.

Please let me know if there are any questions or if there are any flaws in the logic. Thank you.

@liferay-continuous-integration
Copy link
Collaborator

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.

@jonathanmccann
Copy link
Author

ci:test:sf

@jonathanmccann
Copy link
Author

ci:test:relevant

@ruben-pulido ruben-pulido added the 🔍 Backend Review Needed Backend code needs to be reviewed by a member of the team. label Jun 15, 2021
@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 12 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: c7cba8afd61e71c043f0194969306b48597c4600

Sender Branch:

Branch Name: LPS-134264
Branch GIT ID: 7e02cff91f887065d35a52149f6b841ae1ce9e3e

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 10 out of 10 jobs passed

❌ ci:test:relevant - 20 out of 24 jobs passed in 2 hours 18 minutes

Click here for more details.

This pull is eligible for reevaluation. When this upstream build has completed, using the following CI command will compare this pull request result against a more recent upstream result:

ci:reevaluate:1177401_7844

Base Branch:

Branch Name: master
Branch GIT ID: c7cba8afd61e71c043f0194969306b48597c4600

Upstream Comparison:

Branch GIT ID: 6b6ee5210c81ef20c431c93a2624991e93fff5a0
Jenkins Build URL: Acceptance Upstream DXP (master) #1996

ci:test:stable - 10 out of 10 jobs PASSED
10 Successful Jobs:
ci:test:relevant - 20 out of 24 jobs PASSED
20 Successful Jobs:
For more details click here.

Failures unique to this pull:

  2. test-portal-acceptance-pullrequest-batch(master)/unit-jdk8/0
    Job Results:

    2638 Tests Passed.
    1 Test Failed.

    1. AXIS_VARIABLE=0,label_exp=!master #465548
      1. com.liferay.portal.tools.ConfigurationEnvBuilderTest.testBuildContent
        junit.framework.AssertionFailedError: Run "ant generate-config-env" to regenerate modules/configuration-env.txt. expected:<...ay.asset.categories.[admin.web.internal.configuration.FFAssetCategoriesAdminWebConfiguration_setDisplayPageTemplateEnabled
#
LIFERAY_CONFIGURATION_OVERRIDE_COM_PERIOD_LIFERAY_PERIOD_ASSET_PERIOD_CATEGORIES_PERIOD_ADMIN_PERIOD_WEB_PERIOD_INTERNAL_PERIOD_CONFIGURATION_PERIOD__UPPERCASEF__UPPERCASEF__UPPERCASEA_SSET_UPPERCASEC_ATEGORIES_UPPERCASEA_DMIN_UPPERCASEW_EB_UPPERCASEC_ONFIGURATION_UNDERLINE_SET_UPPERCASED_ISPLAY_UPPERCASEP_AGE_UPPERCASET_EMPLATE_UPPERCASEE_NABLED

        

        com.liferay.asset.categories.]configuration.AssetC...> but was:<...ay.asset.categories.[]configuration.AssetC...>
        

        at com.liferay.portal.tools.ConfigurationEnvBuilderTest.testBuildContent(ConfigurationEnvBuilderTest.java:81)
at com.liferay.portal.kernel.test.rule.AbstractTestRule$2.evaluate(AbstractTestRule.java:99)
at com.liferay.portal.kernel.test.rule.AbstractTestRule$1.evaluate(AbstractTestRule.java:59)
Failures in common with acceptance upstream results at 6b6ee52:

@liferay-continuous-integration
Copy link
Collaborator

@ruben-pulido ruben-pulido self-assigned this Jun 16, 2021
@ruben-pulido
Copy link
Collaborator

ci:test:relevant

@ruben-pulido
Copy link
Collaborator

Just started reviewing :)

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 10 out of 10 jobs passed

❌ ci:test:relevant - 21 out of 24 jobs passed in 3 hours 11 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 0a602240c817010ba021c3109da081a00c6af2aa

Upstream Comparison:

Branch GIT ID: 0a602240c817010ba021c3109da081a00c6af2aa
Jenkins Build URL: Acceptance Upstream DXP (master) #2000

ci:test:stable - 10 out of 10 jobs PASSED
10 Successful Jobs:
ci:test:relevant - 21 out of 24 jobs PASSED
21 Successful Jobs:
For more details click here.

Failures unique to this pull:

Failures in common with acceptance upstream results at 0a60224:
  1. test-portal-acceptance-pullrequest-batch(master)/unit-jdk8/0
    Job Results:

    2638 Tests Passed.
    1 Test Failed.

    1. AXIS_VARIABLE=0,label_exp=!master #339604
      1. com.liferay.portal.tools.ConfigurationEnvBuilderTest.testBuildContent
        junit.framework.AssertionFailedError: Run "ant generate-config-env" to regenerate modules/configuration-env.txt. expected:<...ay.asset.categories.[admin.web.internal.configuration.FFAssetCategoriesAdminWebConfiguration_setDisplayPageTemplateEnabled
#
LIFERAY_CONFIGURATION_OVERRIDE_COM_PERIOD_LIFERAY_PERIOD_ASSET_PERIOD_CATEGORIES_PERIOD_ADMIN_PERIOD_WEB_PERIOD_INTERNAL_PERIOD_CONFIGURATION_PERIOD__UPPERCASEF__UPPERCASEF__UPPERCASEA_SSET_UPPERCASEC_ATEGORIES_UPPERCASEA_DMIN_UPPERCASEW_EB_UPPERCASEC_ONFIGURATION_UNDERLINE_SET_UPPERCASED_ISPLAY_UPPERCASEP_AGE_UPPERCASET_EMPLATE_UPPERCASEE_NABLED

        

        com.liferay.asset.categories.]configuration.AssetC...> but was:<...ay.asset.categories.[]configuration.AssetC...>
        

        at com.liferay.portal.tools.ConfigurationEnvBuilderTest.testBuildContent(ConfigurationEnvBuilderTest.java:81)
at com.liferay.portal.kernel.test.rule.AbstractTestRule$2.evaluate(AbstractTestRule.java:99)
at com.liferay.portal.kernel.test.rule.AbstractTestRule$1.evaluate(AbstractTestRule.java:59)

@liferay-continuous-integration
Copy link
Collaborator

@jonathanmccann
Copy link
Author

@ruben-pulido Thank you for letting me know. I've pushed up some changes to resolve that issue. I'll keep an eye on the test results to see if anything still doesn't pass.

@jonathanmccann
Copy link
Author

ci:test:sf

@jonathanmccann
Copy link
Author

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 5b0dd2a25496279a49af5a16f62f9170e63663ac

Sender Branch:

Branch Name: LPS-134264
Branch GIT ID: 648dba95ab0592364c4e9163dcda4277b7991345

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:stable - 10 out of 10 jobs passed

✔️ ci:test:relevant - 23 out of 24 jobs passed in 3 hours 32 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 5b0dd2a25496279a49af5a16f62f9170e63663ac

Upstream Comparison:

Branch GIT ID: 7cd2901e5c4f9c48b954eb82b675e9534a7434ca
Jenkins Build URL: Acceptance Upstream DXP (master) #2002

ci:test:stable - 10 out of 10 jobs PASSED
10 Successful Jobs:
ci:test:relevant - 22 out of 24 jobs PASSED
22 Successful Jobs:
For more details click here.

This pull contains no unique failures.

Failures in common with acceptance upstream results at 7cd2901:
  1. test-portal-acceptance-pullrequest-batch(master)/unit-jdk8/0
    Job Results:

    2638 Tests Passed.
    1 Test Failed.

    1. AXIS_VARIABLE=0,label_exp=!master #464803
      1. com.liferay.portal.tools.ConfigurationEnvBuilderTest.testBuildContent
        junit.framework.AssertionFailedError: Run "ant generate-config-env" to regenerate modules/configuration-env.txt. expected:<...ay.asset.categories.[admin.web.internal.configuration.FFAssetCategoriesAdminWebConfiguration_setDisplayPageTemplateEnabled
#
LIFERAY_CONFIGURATION_OVERRIDE_COM_PERIOD_LIFERAY_PERIOD_ASSET_PERIOD_CATEGORIES_PERIOD_ADMIN_PERIOD_WEB_PERIOD_INTERNAL_PERIOD_CONFIGURATION_PERIOD__UPPERCASEF__UPPERCASEF__UPPERCASEA_SSET_UPPERCASEC_ATEGORIES_UPPERCASEA_DMIN_UPPERCASEW_EB_UPPERCASEC_ONFIGURATION_UNDERLINE_SET_UPPERCASED_ISPLAY_UPPERCASEP_AGE_UPPERCASET_EMPLATE_UPPERCASEE_NABLED

        

        com.liferay.asset.categories.]configuration.AssetC...> but was:<...ay.asset.categories.[]configuration.AssetC...>
        

        at com.liferay.portal.tools.ConfigurationEnvBuilderTest.testBuildContent(ConfigurationEnvBuilderTest.java:81)
at com.liferay.portal.kernel.test.rule.AbstractTestRule$2.evaluate(AbstractTestRule.java:99)
at com.liferay.portal.kernel.test.rule.AbstractTestRule$1.evaluate(AbstractTestRule.java:59)

@liferay-continuous-integration
Copy link
Collaborator

@ealonso
Copy link
Collaborator

ealonso commented Jun 17, 2021

Just started reviewing :)

:octocat: Sent from GH.

@ruben-pulido ruben-pulido removed the 🛠 Changes Required Code has been reviewed and needs changes. label Jun 17, 2021
@ruben-pulido
Copy link
Collaborator

Thanks for the updated changes @jonathanmccann.
The issue is with your changes no longer reproducible and no tests are failing :)

For subsequent pulls I have two suggestions that would help us during the review:

  • I find it helpful to have a separate commit for changes that are only refactoring existing code. We usually prefix the commit message with "Refactor". In this Pull, a first commit could be just extracting the already existing code to the new method _filterGroups and calling the new method from where the existing code was removed. A second commit would then move the assignment of actionId, add the conditional call to _filterGroups and possibly the change in SiteBrowserDisplayContext (that could be also part of a third commit).

  • I personally favour closing an existing PR and opening a new one whenever there are changes (rather than updating it). That makes it easier to compare the changes across PRs.

Thanks!! :)

@ruben-pulido
Copy link
Collaborator

ci:forward

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:relevant
  •     ci:test:sf

The pull request will automatically be forwarded to the user brianchandotcom if the following test suites pass:

  •     ci:test:relevant
  •     ci:test:sf
  •     ci:test:stable

@liferay-continuous-integration
Copy link
Collaborator

Skipping previously passed test suites:
ci:test:relevant
ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

All required test suite(s) passed.
Forwarding pull request to brianchandotcom.
Console

Closed
@liferay-continuous-integration
Copy link
Collaborator

Pull request has been successfully forwarded to brianchandotcom#103363
Console

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@ruben-pulido ruben-pulido

Labels
🔍 Backend Review Needed Backend code needs to be reviewed by a member of the team. ci:test:relevant - success ci:test:sf - success ci:test:stable - success
Projects
None yet
Milestone
No milestone
4 participants
@jonathanmccann @liferay-continuous-integration @ruben-pulido @ealonso