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

Fix for #37: Make it work while restricting domain. #38

Merged
merged 5 commits into from Dec 6, 2019

Conversation

@polx
Copy link

polx commented Dec 6, 2019

Proposed change to fix #37.

Copy link

mflorea left a comment

Just some minor remarks. I can't comment on the actual changes, I trust you on that :)

src/main/resources/GoogleApps/Groovy.groovy Outdated Show resolved Hide resolved
src/main/resources/GoogleApps/Groovy.groovy Outdated Show resolved Hide resolved
src/main/resources/GoogleApps/Groovy.groovy Outdated Show resolved Hide resolved
def wikiUserList = services.query.xwql("from doc.object(GoogleApps.GoogleAppsAuthClass) as auth where auth.id=:id").bindValue("id", id).execute()
if ((wikiUserList==null) || (wikiUserList.size()==0))
wikiUserList = services.query.xwql("from doc.object(XWiki.XWikiUsers) as user where user.email=:email").bindValue("email", email).execute()
wikiUserList = services.query.xwql("from doc.object(XWiki.XWikiUsers) as user where user.email=:email").bindValue("email", usersEmailAddress).execute()

if ((wikiUserList==null) || (wikiUserList.size()==0)) {

This comment has been minimized.

Copy link
@acotiuga

acotiuga Dec 6, 2019

Collaborator

This test is duplicated few lines above.

This comment has been minimized.

Copy link
@acotiuga

acotiuga Dec 6, 2019

Collaborator

However, the code can be optimized after this fix is applied

This comment has been minimized.

Copy link
@polx

polx Dec 6, 2019

Author

I suggest indeed to make these optimizations in the java part.

email = (user.emailAddresses!=null && user.emailAddresses.size()>0) ? user.emailAddresses[0].value : "";
if(usersEmailAddress == "") {
usersEmailAddress = (user.emailAddresses!=null && user.emailAddresses.size()>0) ? user.emailAddresses[0].value : "";
}
def wikiUserList = services.query.xwql("from doc.object(GoogleApps.GoogleAppsAuthClass) as auth where auth.id=:id").bindValue("id", id).execute()
if ((wikiUserList==null) || (wikiUserList.size()==0))

This comment has been minimized.

Copy link
@acotiuga

acotiuga Dec 6, 2019

Collaborator

I'm not sure you will get ever a null from https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-query/xwiki-platform-query-xwql/src/main/java/org/xwiki/query/xwql/internal/XWQLQueryExecutor.java#L79.
It will always be a list, maybe an empty one, but never a null one. Checking for nulls everywhere is unnecessary loading the code. WDYT?

This comment has been minimized.

Copy link
@polx

polx Dec 6, 2019

Author

While I'd agree it's not nice to read, I don't read in the source of XWQLQueryExecutor that this is for sure not null (it returns the result of another variable).
Same remark as above: we should not try to change the groovy code too much now and focus on such enhancements for the java part.

src/main/resources/GoogleApps/Groovy.groovy Outdated Show resolved Hide resolved
if(user.names==null || user.names.size()==0) throw new NullPointerException("Sorry, users without names are not supported.");
def isCreated = xwiki.getXWiki().createUser(xwikiUser, ["first_name" : user.names[0].givenName, "last_name" : user.names[0].familyName, "email" : email, "password" : randomPassword], parentref, null, null, "edit", context.context)
if(user.names==null || user.names.size()==0) {
throw new NullPointerException("Sorry, users without names are not supported.");

This comment has been minimized.

Copy link
@acotiuga

acotiuga Dec 6, 2019

Collaborator

You're trowing NPE knowing that you will catch and check it in other place? I can see that you're used to check for null everywhere, but I don't think is a good idea in trowing this here.
I would rather log something here.

This comment has been minimized.

Copy link
@polx

polx Dec 6, 2019

Author

I think thtat the intent of throwing the NPE here is to transmit a message to the user. Any exception would probably do.
Should I change it to a java.lang.Exception ?

This comment has been minimized.

Copy link
@acotiuga

acotiuga Dec 6, 2019

Collaborator

The idea is to warn with a nice message in the UI and not to break the page with an error. Again would be acceptable for this fix, but we're doing a lot of compromises in rush which is not fine at all. Keep it like that for now.

Paul Libbrecht added 2 commits Dec 6, 2019
@acotiuga acotiuga merged commit 1c04afa into xwikisas:stable-2.4.x Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.