infopages and licenses for neurohub resolves #1010, #1233

[MontrealSergiy]

see #1010, #1233

this work is motivated by request to add a newsletter subscription form, the form is from envoke.com mail campaign server.

current motivation is to add Envoke subscription form, tell me, Bryan or James if need Envoke access, but be careful with it, it has no test environment.

Example html of info page can be provided on request, or downloaded from https://mcgill-my.sharepoint.com/:u:/g/personal/sergiy_boroday_mcgill_ca/ERTHRklcJHpEg_TNA5Y7UfQBKCMSESCyAKoHIINuwTdKpQ?e=Drrcv0

The old wiki on licenses is https://github.com/aces/cbrain/wiki/Custom-Licenses Probably to be modified after the feature is deployed

Note, when replacing new licenses with old on a resource, the information about user signing old licenses might be deleted (I did not remember all quirks of that feature)

[natacha-beck]

Maybe we should rename the :

• neurohub_application_controller.rb to nh_application_controller.rb
• neurohub_portal_controller.rb to nh_portal_controller.rb

To match the other *_controller name.

[natacha-beck]

Not sure to have tested it properly, but seems not to work as expected on my side.

Here is what I have:

1. In licenses, I have a licence for CBRAIN, a licencee for NH and an info for NH:

../cbrain/BrainPortal/public$ tree licenses/ licenses/ ├── cbrain_citation.html ├── neurohub │   ├── nh_citation.html │   └── nh_mail_info.html └── README.txt

2. Then, I created a new user:

#<NormalUser:0x00007f56184039c0 id: 32, full_name: "test_l2", position: "", affiliation: "", login: "test_l2", email: "[email protected]", type: "NormalUser", crypted_password: "XXXXX", salt: "XXX", created_at: Thu, 19 May 2022 19:12:46 UTC +00:00, updated_at: Thu, 19 May 2022 19:12:46 UTC +00:00, site_id: nil, password_reset: false, time_zone: "", city: "", country: "", last_connected_at: nil, account_locked: false, zenodo_main_token: nil, zenodo_sandbox_token: nil>

3. I login in CBRAIN with my new user --> No license show up. But in my console I get:

p l.all_licenses_signed;p l.neurohub_licenses_signed MetaDataStore Load (0.4ms) SELECT meta_data_store.* FROM meta_data_store WHERE meta_data_store.ar_id = 32 AND meta_data_store.ar_table_name = 'users' "yes" MetaDataStore Load (0.2ms) SELECT meta_data_store.* FROM meta_data_store WHERE meta_data_store.ar_id = 32 AND meta_data_store.ar_table_name = 'users' nil

4. I login in NH with my new user --> No license show up But in my console I get:

p l.all_licenses_signed;p l.neurohub_licenses_signed MetaDataStore Load (1.6ms) SELECT meta_data_store.* FROM meta_data_store WHERE meta_data_store.ar_id = 32 AND meta_data_store.ar_table_name = 'users' "yes" MetaDataStore Load (0.9ms) SELECT meta_data_store.* FROM meta_data_store WHERE meta_data_store.ar_id = 32 AND meta_data_store.ar_table_name = 'users' "yes"

[MontrealSergiy]

@natacha-beck

Not sure to have tested it properly, but seems not to work as expected on my side.

Here is what I have:

1. In `licenses`, I have a licence for CBRAIN, a licencee for NH and an info for NH:

Natacha, strangely I cannot reproduce the bug. Are you sure that you created a new user and not recycled existing one? Did you configure the Portal server license field properly

because new users work well for me nh_license_infopage

[natacha-beck]

@MontrealSergiy never mind I put the license in the right place but forgot to setup with the interface.

[natacha-beck]

I already spot 2 issues:

1. I create a new user, then login. When: a. I going to the NH account. b. I am redirecting to the license page. c. I try to logout. d. I am stuck to the license page, without the ability of logout.

2. I create a new user, then login. When: a. I going to the NH account. b. I am redirecting to the license page. c. I click on I do not agree d. I am redirected to the CBRAIN login page not the NH login page.

[MontrealSergiy]

@natacha-beck pull the latest code for these issues

[natacha-beck]

Just a quick question, not sure it is the way it should be.

If you have a _info page with a checkbox, do you need to check the box to agree or just click on the text I have read this...

[MontrealSergiy]

for _info page no special checkbox logic is foreseen I just do not see need, because it is info page, the less red tape the better. Do you think it might be of some use ?

[natacha-beck]

Not sure. The *.html file that is used for the _info page can include a checkbox. Since it is an arbitrary *.html file. Then it feels strange to have a checkbox without the need of check this one.

[MontrealSergiy]

I see notion of info page a contradictory to elaborated checks. Checkbox checks seems utterly useless at the moment, without any use case. I would go with Yagni approach until situation. Generally specking, the form on info page is likely have check-boxes unrelated to CBRAIN but another website, such as a maillist server, so I would like avoid any risk here. Personally, I would not even require to click 'continue', though it would diverge from Pierre original suggestion. So I guess my aversion to checkbox checks is as I expect stakeholders eventually ask to relax even Continue box logic. Yet, checking check-boxes might makes code slightly simple, so if you insist I could do it, just confirm.

[natacha-beck]

@MontrealSergiy and @bryancaron is it still needed ? I think we have what was needed for the Newsletter ? Not sure if this PR add something else we need ?

[prioux]

@natacha-beck I'm going to review this.

[prioux]

@MontrealSergiy Can you please rebase this PR? It's conflicting with the current dev branch.

[MontrealSergiy]

rebased

[prioux]

I'm going to write down behavior bugs as I experience them.

1- licenses seem to be automatically signed if the users switches from CB to NH and back to CB a) as admin add a license file for cbrain and add the license name to the portal's config b) as a normal user, access cbrain, you will be prompted with the license c) instead of clicking the checkbox, click on the neurohub link at the top right d) now in NH, click back to the CB link at top right e) you are back in CBRAIN and the meta data for the user account says "all_license_signed" => "yes" even if the new license names is not in the list of signed licenses.

[prioux]

Renaming neurohub_application_controller into nh_application_controller was not part of the goal of this project. If we want to do that, we'll do it in a separate PR. Please revert all these name changes.

[prioux]

I don't understand what your description of this PR (at the very top of the page) says Note, for an existing user to get effect of new license, the property user.all_licenses_signed or neurohub_licenses_signed show be reset to nil when your code already seem to do that, at line 106 of

BrainPortal/lib/license_agreements.rb

[MontrealSergiy]

Renaming neurohub_application_controller into nh_application_controller was not part of the goal of this project. If we want to do that, we'll do it in a separate PR. Please revert all these name changes.

that's what @natacha-beck asked, and nobody objected at the time https://github.com/aces/cbrain/pull/1234#issuecomment-1131960489

[prioux]

There are lots of places where you completely ignore the CBRAIN/Ruby style conventions. e.g.

def cbrain_license_agreement_set # cbrain required licenses
    license_agreement_set.reject {|l| l.include?('/')}
end

You put a comment describing the method ON the def line instead of above it. On the def line you're supposed to put #:nodoc: if the method is not supposed to be documented for others developers (e.g. if they are not expected to invoke that method), and nothing if the documentation put above the def line contains useful information for developers (e.g. when you are writing code for them to use).

[MontrealSergiy]

Maybe we should rename the :

• neurohub_application_controller.rb to nh_application_controller.rb
• neurohub_portal_controller.rb to nh_portal_controller.rb

To match the other *_controller name.

Hi @natacha-beck @bryancaron , Pierre did not like the cramping this controller name refactoring into this PR, so if you do not mind I am rolling back to 'nh'

[MontrealSergiy]

comments are addressed

PS. I noted that CBRAIN does not show message "CBRAIN cannot be used without signing the End User Licence Agreement" is not shown, if needed I can correct but that would require session controller fixes. Or maybe just delete the message? (2023 update - I might a bit forgot the context but what I meant I found some seemingly long dead code/message, yet not sure shall I reanimate it, cut, or leave it as is)

[prioux]

I found many bugs. I'll document them one by one in separate post. First, I'll describe here my testing setup:

1. in BrainPortal/public/licences I created a subdirectory called 'neurohub'
2. in that directory I created three license files:

File: a-box.html

<h1>With Checkbox</h1>

NH License text

<input name="num_checkboxes" type="hidden" value="1">
Check this box to continue: <input name="license_check_1" type="checkbox" value="agree" /><br>

File: b-plain.html

<h1>No Checkbox</h1>

NH License text

File: c-plain_info.html

<h1>Info No Checkbox</h1>

NH License info

3. I keep a browser window connected as an admin user, and I add the following three licenses to the license box:

neurohub/a-box
neurohub/b-plain
neurohub/c-plain_info

4. I keep a console open with a variable set to point to a normal user that I will use for my tests:

rails> u=User.find(6) # adjust
rails> u.meta.reload;u.meta[:signed_license_agreements]=nil;u.meta[:all_licenses_signed]=nil;u.meta[:neurohub_licenses_signed]=nil

This last line with four ruby statements I keep around, ready to zap the user's state just by doing up arrow and return.

5. In a separate browser window (using incognito mode or a different browser) I connect to the portal on the NeuroHub login page at https://localhost:3000/signin

I will document the bugs now in new posts.

[prioux]

BUG-1

The first license shown can be agreed to by clicking any of the two checkboxes (the one in the license text or the one provided by the NH interface). The two checkboxes don't even have the same name=.

[prioux]

BUG-2

The layout of the boxes that show the licenses is all wrong. I suspect badly structured HTML. Also the top word 'License' doesn't seem to be in the font/style mandated by Candice's layout guidelines.

[prioux]

BUG-3

I can agree on the second license (b-plain) without even checking the checkbox.

[prioux]

BUG-4

I can find no way for a user to review the licenses they have signed. There are no links back to read them again.

If I try to manually go to http://localhost:3000/nh_show_license/neurohub/a-box I get an exception raised.

[prioux]

BUG-5

In fact, going to http://localhost:3000/nh_show_license/SOMETHING can result in all sort of weird things, including when the SOMETHING is a normal CBRAIN license or a prefixed neurohub/SOMETHING

[prioux]

BUG-6

The url http://localhost:3000/nh_show_license/neurohub/c-plain_info show the license but with a button on the side that does nothing.

[prioux]

BUG-8

In the controller code, there is no validation at all that the value params[:license] is a valid license identifier. This is close to introducing security bugs with arbitrary path access, given that you changed the routes file to perform path globbing after the action keyword:

unix> rake routes | grep license
                                 GET    /show_license/*license(.:format)                 portal#show_license
                                 POST   /sign_license/*license(.:format)                 portal#sign_license
                                 GET    /nh_show_license/*license(.:format)              neurohub_portal#show_license
                                 POST   /nh_sign_license/*license(.:format)              neurohub_portal#nh_sign_license

Please change the routing system to return to the standard single token matching in the routes (:licence instead of *license)

[prioux]

I will post more comments in the code now.