[stable10] fix multi gdrive external storage bug #34987
Conversation
karakayasemi
changed the title
|
something failed in js tests. I will look at it. |
Codecov Report
@@ Coverage Diff @@
## stable10 #34987 +/- ##
===============================================
- Coverage 64.29% 44.97% -19.33%
===============================================
Files 1285 116 -1169
Lines 76824 11565 -65259
Branches 1307 1307
===============================================
- Hits 49394 5201 -44193
+ Misses 27049 5983 -21066
Partials 381 381
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## stable10 #34987 +/- ##
==============================================
- Coverage 64.29% 64.29% -0.01%
Complexity 20040 20040
==============================================
Files 1285 1285
Lines 76824 76824
Branches 1307 1307
==============================================
- Hits 49394 49393 -1
- Misses 27049 27050 +1
Partials 381 381
Continue to review full report at Codecov.
|
|
This requires careful testing and I don't think I have the environment to test this PR. I'll skip my worries about optimization since they're probably quite small (we might be assigning different function instances for each storage, increasing memory usage), and I'm not a js expert, so we can ignore those. The code looks fine, but I don't know if it works in all the possible scenarios. |
|
Since I only changed gdrive.js, I do not expect any regression in the other external storage providers.
|
|
Any guarantee about the loading order of the gdrive.js and settings.js files? If there is no guarantee that the files will be loaded in a specific order, I'm not sure if there could be problems. |
|
I'm not sure 100% about this, but I guess the process goes as described below:
The reason this is working is because the "selectBackend" event, which is the one the backend is registering for with the "whenSelectBackend", is triggered in the "newStorage" function which is called as a success callback for an ajax call during the "loadStorage". Quite a difficult explanation to answer my question on #34987 (comment) |
|
@karakayasemi in case of multiple gdrive accounts, could you doble-check that there is only one request being made to the backend for the oauth events? |
| * abstract away the details of sending the request to backend for getting the AuthURL or | ||
| * verifying the code, mounting the storage config etc | ||
| */ | ||
| $('.configuration').on('oauth_step1', function (event, data) { |
There was a problem hiding this comment.
Actually, the old code is registering listeners when document.ready() for all .configuration class elements. Whenever oauth_step1 event triggered, all .configuration class elements are receiving this event and the below line (backend_id check) is preventing multiple ouath requests.
The problem here is new elements are not listening oauth_step1 event, because of that grant access button is only working for elements that rendered before document.ready()
| * abstract away the details of sending the request to backend for getting the AuthURL or | ||
| * verifying the code, mounting the storage config etc | ||
| */ | ||
| $tr.find('.configuration').on('oauth_step1', function (event, data) { |
There was a problem hiding this comment.
With this PR, we are registering oauth_step events for only related<tr> element when user select backend. IMHO, this way is safer than previous.
|
Yes, I tried one more time with adding 5 gdrive accounts, oauth events only triggered for related |
|
I do not see this in |
Description
oauth2_step1andoauth_step2handlers are always registering for the first container ingdrive.js. I moved them inside ofwhenSelectBackendevent and registered each of them separately.@op6sie, @Lirein please test if you available.
Related Issue
fixes #31154 , fixes #31497
Motivation and Context
Fixing bugs.
How Has This Been Tested?
Try to add multiple gdrive external storage. It should work.
Types of changes
Checklist:
Open Tasks: