Skip to content

Fix duplicated storage mounts in the settings view during oAuth2 flow#29145

Merged
PVince81 merged 2 commits into
masterfrom
dupped_mounts
Oct 16, 2017
Merged

Fix duplicated storage mounts in the settings view during oAuth2 flow#29145
PVince81 merged 2 commits into
masterfrom
dupped_mounts

Conversation

@jvillafanez

@jvillafanez jvillafanez commented Oct 4, 2017

Copy link
Copy Markdown
Member

Description

Fix duplicated mount that could happen during oAuth2 flow. This has been partially tested with google drive but should fix other oAuth2 storages. Note that oAuth1 storages will likely present the same problem, which isn't fixed by this PR.

Related Issue

#28479

Motivation and Context

Duplicated mounts shouldn't be saved

How Has This Been Tested?

Manual testing

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.

@jvillafanez

Copy link
Copy Markdown
Member Author

As a side note, the "grant access" button will be disabled to prevent further clicks

) {
$('.configuration').trigger('oauth_step2', [{
$tr.find('.configuration').trigger('oauth_step2', [{
backend_id: $tr.attr('class'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh okay so you are finding the configuration box closest to the current row to prevent any conflicts right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Only one row should trigger the step, no more. Finding the configuration box from the row seems the easiest and most understandable approach.
Btw, I'm not sure why the configuration box is triggering events when it's more natural that a button triggers them, but anyway this is focused on fixing the issue not changing anything.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Button can trigger step 1 but not step 2 which is fired after OAuth completion

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Button can trigger step 1 but not step 2 which is fired after OAuth completion

Any reason for that? It's technically possible. Whether it's a good or bad idea, that's another topic to discuss.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is how it was built
Originally instead of firing events it was authenticating only Google Drive as part of OAuth2 and there was no way for other apps to know when OAuth was completed so instead I triggered oauth2 events which respective apps could listen

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@jvillafanez I think this would let an app's code decide whether they use a button or anything else for triggering the oauth flow.

@PVince81 PVince81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 39eb79c into master Oct 16, 2017
@PVince81 PVince81 deleted the dupped_mounts branch October 16, 2017 08:13
@PVince81

Copy link
Copy Markdown
Contributor

@jvillafanez please backport to stable10

@jvillafanez

Copy link
Copy Markdown
Member Author

Backport to stable10: #29254

@davitol

davitol commented Dec 1, 2017

Copy link
Copy Markdown
Contributor

After this fix, i've seen duplicated WND mounts when configuring it. Maybe a race condition... will look at it a bit more, due to it happens once in a blue moon.

@lock

lock Bot commented Aug 1, 2019

Copy link
Copy Markdown

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 1, 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