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

LPS-75193 Announcements portlet's Distribution Scope is "General" regardless where it is placed #612

Closed
wants to merge 5,382 commits into from

Conversation

IstvanD
Copy link

@IstvanD IstvanD commented Oct 18, 2017

Hey @robertoDiaz

I applied logic directly from the Social Office Announcements portlet to the Announcements portlet to auto-select the distribution scope based on the Site the portlet is placed.

Could you please review it?

Thank you,
István

4lejandrito and others added 30 commits October 16, 2017 16:30
…guration values in RecentDocumentsMessageListener
…endent module, in order to enable or disable we need to enable or disable the module, but we don't need a property to do that
…creating a wrapper in documents and media to use the new modules
…creating a wrapper in documents and media to use the new modules
tomwang2011 and others added 23 commits October 17, 2017 20:11
…et in order to prevent naming conflict with the "group" variable defined in init.jsp
@liferay-continuous-integration
Copy link
Collaborator

@liferay-continuous-integration
Copy link
Collaborator

Some tests FAILED.

Build Time: 2 hours 19 minutes 12 seconds 977 ms

Base Branch:

Branch Name: master
Branch GIT ID: 31c306fad21ee0718ef3597a9cda6a3212a569a1

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

Failures unique to this pull:

  1. test-portal-acceptance-pullrequest(master)
    Job Results:

    116 Jobs Passed.
    1 Job Failed.

    Downstream jobs FAILED.

For upstream results, click here.

@robertoDiaz
Copy link
Owner

Just started reviewing :)

:octocat: Sent from GH.

@robertoDiaz
Copy link
Owner

Hi @IstvanD

The code looks perfect but we are not going to introduce this in master:

After talk about the expected behavior with @sergiogonzalez we have agreed that in this case, change the current behaviour could be a bit confusing, since this is an arbitrary decision.

For example, we decided to use the current group, but following the same reasoning, why don't we use the role of the current user?

Since that we tend to think that the general scope is the most accurate as default scope, so we think that we have nothing to fix in master, and we should backport this behaviour to 7.0.x.

Can you send me a PR to the ee repo with this changes??

Thanks!

@IstvanD
Copy link
Author

IstvanD commented Oct 24, 2017

Hi @robertoDiaz

I've send the pull https://github.com/robertoDiaz/liferay-portal-ee/pull/49.
Thanks for the suggestion.

@robertoDiaz
Copy link
Owner

@IstvanD

Thanks to you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet