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

OAUTH2-214 Semantic versioning #118

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

csierra
Copy link

@csierra csierra commented Aug 2, 2018

/cc @topolik

This is how I believe we should do it in master, with the new update method.

Let me know your thoughts or if I broke something.

stian-sigvartsen and others added 12 commits July 25, 2018 17:23
…e only if they resolve to the same ScopeGrants
…ScopeAliases() so it is also persisted as a sorted list + Ignore blank scope aliasees for optimized persistence
Implement the logic in the `update` method. This is more easily
backporteable and also we don't make other service responsible of the
state of this service
…e.updateScopeAliases() so it is also persisted as a sorted list + Ignore blank scope aliasees for optimized persistence"

This can't depend on the caller. It is part of the service API to store
it sorted
@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.

@csierra
Copy link
Author

csierra commented Aug 2, 2018

ci:test:sf

@csierra
Copy link
Author

csierra commented Aug 2, 2018

ci:test:relevant

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 2 minutes 30 seconds 566 ms

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 7522ba6089354225ead164c07fa79eb87735e3c5

Sender Branch:

Branch Name: pr-577-tomas-master
Branch GIT ID: 427653a72b22578fc81fa9793933449e002dc78c

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

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:relevant - 21 out of 21 jobs passed in 49 minutes 35 seconds 898 ms

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 7522ba6089354225ead164c07fa79eb87735e3c5

Copied in Private Modules Branch:

Branch Name: master-private
Branch GIT ID: 9ddf558ed78e4fdd7985f7bfac6318297205d1b3

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

@stian-sigvartsen
Copy link
Owner

I am wondering if it is confusing to have updateApplicationScopeAliases and updateScopeAliases() method in the two respective services. They have very similar parameters, and the only difference is the former will update the OAuth2Application with the modified date & foreign key.

The more I look at it I feel the one in OAuth2ApplicationLocalService should just receive the foreign key and no List. Essentially moving the service orchestration out of the service layer and into the MVCActionCommand.

Also I am wondering if we really need applicationId when creating OAuth2ApplicationScopeAliases. I mean multiple OAuth2Applications can share them? The only benefit I see right now is making it easier to tidy up / delete them when an OAuth2Application is deleted. @topolik?

@stian-sigvartsen
Copy link
Owner

On second thought, I see the OAuth2ApplicationScopeAliasesLocalService doesn't have a remote counterpart, so maybe there would be no confusion between the similarly named methods. OAuth2ApplicationScopeAliasesLocalService is just an implementation detail in other words.

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

Successfully merging this pull request may close these issues.

3 participants