Updating the list of users in a workgroup (Bug1+Bug2)

[natacha-beck]

I was annoyed by this issue and couldn't ignore it.

The fix concerns bug 1 for the moment:

the users already members of the project don't have their checkboxes pre-selected in table A

Note: the select_all_box is also used in bourreaux/rr_disk_usage. Seems to be a little bit buggy too in rr_disk_usage when you:

• select_all a row then select_all a column (see screenshot 1)
• then unselect the row did not unable the all column (see screenshot 2).

Then

[natacha-beck]

@MontrealSergiy please have a look to the following fix and test it.

[prioux]

I don't like the fact that we must add data-noload whenever we use our helper. The point of a helper is to make things easy for the UI developer.

[MontrealSergiy]

@natacha-beck Hi Natacha, will do, yet could you please explain why do you need emulate click on load event at all? I fail to follow, is it for some special cases of "persistent" check-boxes only?

[prioux]

Not only that, this new option is secret, it's not even documented in the usage statement of

def select_all_checkbox

[MontrealSergiy]

On the bright side Bug1 addressed - current users shown after the click on the edit (though select_all boxes are not)

[MontrealSergiy]

for new issue (intersecting master selects) you mentioned a quick fix could be use on change even instead on click. Yet I bet recomputing everything might be faster in some corner cases

[natacha-beck]

The fact that we called click_select_all on load made the checkbox of the active user (table) dependant of the status of WorkGroup/SiteGroup selection. That's was the reason why no user was selected (previous member of the group was ignored), and why I introduce the noload option.

Seems to work properly I will have a closer look, the logic here seems strange when it apply to the project group. Maybe due to a mix between user information and WorkGroup/SiteGroup information. That confirm @prioux suspicion about:

I suspect there is a confusion about the IDs and classes associated with the normal checkboxes in table A and the master select checkboxes in table B.

[MontrealSergiy]

@natacha-beck I was just wondering if you must at all, why do you, on load, define subordinate from master and not vise versa? Are there any special use cases? Perhaps related to checkbox persistence?

[MontrealSergiy]

I do not think Bug 2 reported by Pierre is strange or some id confusion. The group member edit section just follows the logic you implemented (masters mirror the state of subordinates). Well, the issue you mentioned in the beginning of PR is most likely also present on group edit though less annoying than on that rr_disk_usage table. And I noted a minor Bug 4 : the invite user pop up works almost like it has normal simple master chekboxes (subordinate does not affect master ).

[MontrealSergiy]

A side note with your changes master-slave terminology probably does not apply any more. Maybe Leader/Group Lead checkbox? Manager? Representative checkbox? Delegate checkbox? Elder checkbox? Head? Principal? Chieftain? Foreman? :)

[prioux]

There is too much talking here. I just want a solution. Make it all work like before the bug occurred. No programming shenanigans. Keep things simple. The select_all_checkbox() helper was committed by Tarek in Nov 2011, I don't want programmers to no longer be able to use it as it was designed.

[MontrealSergiy]

I am not sure does Pierre expect a total rollback or just minor polishing, but your PR is a bit suboptimal either way. It is customary to require additional fancy param to enable some new bell and whistles and keep old function signature working as before. Demanding new params just to keep old code work well is strange. Normally you add special data-onload class, rather than noload, or even more explicitly data-reset-checkboxes-to-master-values . Perhaps you do not even need that but special bandaid to address the cases of persistence or/and default checked masterbox

[MontrealSergiy]

Natacha groups Bug 1 seem addressed by new version too, not sure how macaques should work never tried it before. Will install and test tomorrow. The "bid app handler" seems suspicious, not sure how other checkboxes might be affected. Please mention in method comment/docs that checkboxes will be reinitialized to master value. Maybe keep copy of old branch we do not which would be preferred, Pierre mentioned that 'persistance' is not conceptually imply need to re-initialize slave to master value.

[natacha-beck]

It fix both bug.

[MontrealSergiy]

Confirm Bug 1 and so-called Bug 2 addressed.

[MontrealSergiy]

the difference in select_all_checkbox method behaviour for persistent case could be documented better ( the fact that for persistant case you reinit subordinate checkboxes to select all box value, even if they already have checked property and principal has not is still surprising, best to hint on it if not explain in details)

[prioux]

I don't understand why you both say that the bug is fixed now, when clearly it still doesn't work?

[MontrealSergiy]

you mean Bug1 Bug2 or something else'

[MontrealSergiy]

do you use safari or chrome?

[prioux]

I've not even gotten to bug 2 yet. As soon as I open any workproject in CBRAIN, the users currently in the project are not pre-selected. Why did you guys say this was fixed?

[prioux]

Firefox.

[MontrealSergiy]

refresh? Ctrl F5

[prioux]

Oh never mind. THanks Sergiy. yes, I needed to do a full refresh

[prioux]

I think Firefox caches Javascript libraries much more aggressively than other browsers

[prioux]

My sincere apologies!

[MontrealSergiy]

I think it is common issue. Some projects may resort to hacks like myjavascript.js?ver=2.3.4a to disable the caching. There are more civilized methods https://stackoverflow.com/questions/118884/how-to-force-browsers-to-reload-cached-css-and-js-files, but more work consuming