Skip to content

Conversation

@jvillafanez
Copy link
Member

Description

Related Issue

https://github.com/owncloud/files_onedrive/issues/25

Motivation and Context

javscript code crashed and prevented oauth2 storages to be granted access as the button was missing

How Has This Been Tested?

Manually with google drive in master, adding an artificial delay to force the issue happen consistenly

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Backport to stable10 will be needed.

@jvillafanez
Copy link
Member Author

Backport to stable10 in #28496

var authMechanism = $(tr).find('.selectAuthMechanism').val();
callback($(tr), authMechanism, self._allAuthMechanisms[authMechanism]['scheme']);
if (authMechanism !== undefined) {
var onCompletion = jQuery.Deferred();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of this deferred/promise ? usually the purpose of a promise is to be returned and resolved later within a callback. Also it seems to be unused.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gdrive uses the "onCompletition" callback, so unless we send something it will explode because the variable will be undefined. From my point of view, it was buggy from the beginning.
It's either use a dummy (the current solution) or try to remove these calls and force triggering an event at some point.

I've also tried to remove the callback. It seemed to work, so I don't know what was the reason to put that code in the first place.

I've already spent too much time just for this, so I've went for patching the bug.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hemant-Mann I believe this might be related to your recent changes ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvillafanez Are you referring to this onCompletion callback?
https://github.com/owncloud/core/blob/master/apps/files_external/js/oauth2.js#L17

@PVince81 I don't think this Issue is related to my recent changes.
Hey @jvillafanez could you please describe the issue in a little more detail

Copy link
Contributor

@Hemant-Mann Hemant-Mann Jul 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvillafanez I have gone through the whole flow of selecting GDrive (when creating a PR) and was able to successfully authenticate it using OAuth2 so I am not sure what's the error

See this screenshot here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delay the https://github.com/owncloud/core/blob/master/apps/files_external/js/oauth2.js#L7 call 5 seconds with a setTimeout, something like:

setTimeout(function(){
  console.log('calling method');
  OCA.External.Settings.mountConfig.whenSelectAuthMechanism(function($tr, authMechanism, scheme, onCompletion) {
    .......
  });
}, 5000);

When you select a new mount gdrive mount, the button to grant access to the storage is missing.
This happens in the personal settings page, when several admin mount are presents for the user.

Note that this affects to other storages that are loaded before the files_external's settings js and are forced to use the "mountConfigLoaded" to register the auth mechanism. That delay intends to simulate that, as the call to that method will be done quite late.

As far as I know, the bug might have been there before @Hemant-Mann 's changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so if @Hemant-Mann has no strong objection let's just merge this...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No Objection from my side. This should be good to go

@PVince81 PVince81 merged commit 3b033ed into master Jul 28, 2017
@PVince81 PVince81 deleted the external_auth_crash branch July 28, 2017 09:15
@lock
Copy link

lock bot commented Aug 3, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants