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]

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

[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.

[prioux]

My review is not finish but I want to send out this first batch of comments.

[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 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.

[prioux]

See comments

[prioux]

I have two more general changes to request:

Request-1

Change the convention for neurohub licenses to be prefixed with "neurohub/" and to be in a subdirectory called "public/licences/neurohub". Instead, the NH licences should simply start with "nh_". E.g. nh_citation.html, nh_email_subscribe_info.html

Request-2

The license input box in the portal's SHOW/EDIT page should have some information for the administrator about the license conventions.

[prioux]

Request-3

All code returned to the original state before the pull request state should stay identical (no changes in white space, blank lines etc). The DIFF page here in the pull request should only show the minimal amount of changes necessary to implement the function of the PR.

[MontrealSergiy]

well, due reiterating requests to refactor something I can factor few lines of sign license action to code a User model, and perhaps, checkbox number checking into a protected method, what do you think?

Request-3

All code returned to the original state before the pull request state should stay identical (no changes in white space, blank lines etc). The DIFF page here in the pull request should only show the minimal amount of changes necessary to implement the function of the PR.

[MontrealSergiy]

@prioux I am trying to rebase to fix conflicts of the new version. Is there a recommended way to handle or regenerate cbrain_file_revisions.csv file ?

[MontrealSergiy]

Problems and requests addressed, I think

I did however a minor a straightforward refactoring, hope it is fine but easy to roll back if you dislike it.

[MontrealSergiy]

I also fixed a project license styling issue reffered at #1318

but can put it in a separate PR if needed

[MontrealSergiy]

@bryancaron Current flow - license, info-subscription page that starts with nh- prefix, and for info/subscription _info suffix are shown to user
after it is a) added by an admin to cbrain/BrainPortal/public/license folder; page can either self containing or embed another page/or html fragment hosted say be envoke.com and then b) added by an admin to the license list .

Users see that subscription/infobox/license when

1. when new user is given credentials and first time logs into NeuroHub with their credentials
2. if they never explicitly logged by to NeuroHub, but are existing CBRAIN users who switched to NeuroHub skin by clicking NeuroHub button
3. all the existing NeuroHub users will be shown that license/infopage/subscription if they will try use NeuroHub interface. Yet they can continue use old CBRAIN interface with impunity.

If you want to add user during NeuroHub signup and (perhaps after email validation), it is best to resuscitate March 2022 API version, https://github.com/MontrealSergiy/cbrain/tree/email, especially if you like to avoid duplicate email validation for the account and for mailing list subscription

UPDATE: as per discussion at NeuroHub meeting April 12, no need for API, at the least as the first step form-based registration is ok.

[xmpham]

Newsletter licence to agree by the user
Sign up to receive our Monthly Newsletter or Event invitations will help you to learn more about NeuroHub and keep yourselves up to date with new development
Edit suggestion: keep you up to date or keep you informed

[MontrealSergiy]

A gentle reminder that the requested changes and problems had been addressed a while ago.

As discussed, license folder is flat while neurohub licenses are marked with nh- prefix (using hyphen rather than underscore)

The verbiage on Envoke list is changes though slightly differently from what Mai asked