Maintainer feedback page and functionality

[joj0s]

This PR adds a page to submit feedback to maintainers. The submission process includes creating a spreadsheet containing potential terms to create/import, and also creating a GitHub issue.

Some notes about this PR:

• It is built on top of PR #77. To make the review process easier, I suggest using branch ols_queries as the base branch. PR #77 should be merged soon, and then I'll rebase this one.
• The Heroku deployment requires some configuration of GitHub and Google APIs to work. It should be up and running by the time this PR is reviewed, but if not, please report it in this PR's comments.

Closes #73

This is deployed in http://trait-curation-2.duckdns.org/traits/

[joj0s]

@tskir There is a problem with this PR that I found out while deploying it on Embassy. For some reason, when I deployed the app to Embassy, the OAuth window for Google Sheets/Drive authentication doesn't open in the user's browser, and there is only a prompt in the server with an OAuth link showing up.

Even if I find a workaround, the problem I found out while researching this issue, is that as far as I understand, when a log in with the Sheets and Drive API is successful, the user's client_id and access token are saved permanently in the server, and the app uses those credentials to authenticate with Google Sheets pretty much permanently. I haven't been able to test it, but this might result in a user creating a spreadsheet with the creds of another user who authenticated before him, without even being prompted to do so.

The alternatives are two: I think the best approach, and even preferable to a regular OAuth flow, is to create a service account for the app. A service account is a special type of Google account intended to represent a non-human user that needs to authenticate and be authorized to access data in Google APIs. I think it makes the most sense to create the sheets under this account. The users wouldn't need to give any access of their Google Drive to the application and I wouldn't have to change the spreadsheet generation code at all, only the authentication section.

The other option is to ditch the gspread library. The problem is that two other libraries that I found, seem to use the same authentication strategy, so the only alternative seems to be to use the Google API directly and not through an external library.

[joj0s]

Update: Creation of a sheet with a service account is indeed successful. Here you can see it: https://docs.google.com/spreadsheets/d/1sgIYCPK6M4-7xMl-gDZiAqQLPjJerdO44xwI86EYXo8/

And the owner looks like this:

Should I go ahead and make the changes to the code (actually only a single line changes), and add instructions for obtaining service account credentials?

[tskir]

Hrm. So it follows that the gspread library does not support multiple users or access keys? That's unfortunate. I was fine with just storing the credentials file on the server, but not with reusing it across different users.

Using a service account seems like a reasonable option; my only concern is who would then have admin control over that table? For example, if we wanted to delete it, how would we go about it?

[joj0s]

Using a service account seems like a reasonable option; my only concern is who would then have admin control over that table? For example, if we wanted to delete it, how would we go about it?

You can specify that when creating the service account. For the one I just created, I gave admin access to everyone with an ebi.ac.uk Google domain account.

[tskir]

You can specify that when creating the service account. For the one I just created, I gave admin access to everyone with an ebi.ac.uk Google domain account.

I tried opening that spreadsheet from my EBI Google account and unfortunately I still have the "Move to bin" option greyed out.

I guess, if necessary, we could remove/administrate the spreadsheets using the service account credentials directly. In this case, we need to make sure that for any given installation the service account credentials are persistent. Looking at gspread documentation, this should be possible like this:

gc = gspread.service_account(filename='path/to/the/downloaded/file.json')

[joj0s]

I tried opening that spreadsheet from my EBI Google account and unfortunately I still have the "Move to bin" option greyed out.

I gave myself access and tried it too with the same result, that is weird, I will look into it.

gc = gspread.service_account(filename='path/to/the/downloaded/file.json')

I am fairly certain that this option just changes the default path that gspread looks for to find the service_account.json file. If we omit that, the service acount creds will still persist in the default folder, which is ~/.config/gspread/service_account.json.

[joj0s]

A good alternative would be to give 'owner' permission to specific people for a spreadsheet using gspread, when I create the spreadsheet. We could follow a similar approach where I give 'owner' permission to everyone with a ebi.ac.uk domain account, or just to the app's admins.

Reference: https://gspread.readthedocs.io/en/latest/api.html#gspread.Client.insert_permission

[tskir]

I am fairly certain that this option just changes the default path that gspread looks for to find the service_account.json file. If we omit that, the service acount creds will still persist in the default folder, which is ~/.config/gspread/service_account.json.

Ah, I see. The location doesn't matter that much, I just hope that it will indeed persist, i.e., that it will not unnecessarily download a new auth JSON every time, including on app restarts.

A good alternative would be to give 'owner' permission to specific people for a spreadsheet using gspread, when I create the spreadsheet. We could follow a similar approach where I give 'owner' permission to everyone with a ebi.ac.uk domain account, or just to the app's admins.

If we could grant ownership access to all app admins, that would be great. Of course, provided that this actually works, since it currently appears not to, at least regarding sheet deletion :-)

[joj0s]

If we could grant ownership access to all app admins, that would be great. Of course, provided that this actually works, since it currently appears not to, at least regarding sheet deletion :-)

So, I am adding 'edit' access to anyone and owner access to EBI acccounts, does that sound good?

Update: I am just going to give access to app admins indeed. @tskir did you get an email notifying that you have ownership of the latest spreadsheet?

[tskir]

So, I am adding 'edit' access to anyone and owner access to EBI acccounts, does that sound good?

Edit access to anyone is good; and owner access should be restricted to app admins, not all EBI accounts on the whole (especially considering that in the future this might have use cases outside of EBI)

@tskir did you get an email notifying that you have ownership of the latest spreadsheet?

No, I'm afraid I didn't get any emails regarding this

[joj0s]

No, I'm afraid I didn't get any emails regarding this

That's really curious, using this code, I got ownership for the latest spreadsheet and I got an email notifying me. Can you confirm I entered your email correctly?

gc.insert_permission(sheet.id, value = 's3apostol@gmail.com', perm_type = 'user', role = 'owner')
gc.insert_permission(sheet.id, value = 'ktsukanov@ebi.ac.uk', perm_type = 'user', role = 'owner')

EDIT: Oh, that's why you didn't get an email:

So in that case, who should I assign as owner? I could assign the person making the request the owner of the file if we don't want to single out an admin as owner of all sheets.

[joj0s]

I updated the sheet creation process. I used batch update to insert all values at once per worksheet. So now I am making four requests in total:

• One to add different formatting to the 'needs_import" worksheet headers
• One to add all the mapping information to the 'needs_import' worksheet
• One to add different formatting to the 'needs_creation worksheet headers
• One to add all the mapping information to the 'needs_creation worksheet

I also updated the deployment of the 'feedback' branch in http://trait-curation-2.duckdns.org/

[joj0s]

@tskir I added transferring of ownership upon successful spreadsheet creation. When you can, please go to http://trait-curation-2.duckdns.org/traits/feedback and try to create an issue and confirm whether you have been made owner of the generated spreadsheet.

[joj0s]

Regarding the spreadsheet ownership approach: unfortunately I can't test it, because for some reason there's no feedback form on the Feedback page. Do you know why this might be?

Yes, this is because I am hiding the form when there are no terms to import. I went ahead and reviewed one from the dummy data so that you can test this

To apply this sanitize function would a good idea (in the future, it can also be reused throughout the project for similar purposes).

I will go with this approach then.

[tskir]

Yes, this is because I am hiding the form when there are no terms to import.

This doesn't seem right: the form should be shown when there is at least one term to either import or create. While rare for our use case, I can definitely imagine a situation where someone would want to request creation of 10 terms and import of none.

If this can be fixed quickly, then we can do this as part of this PR; otherwise we can create a follow-up issue

[joj0s]

If this can be fixed quickly, then we can do this as part of this PR; otherwise we can create a follow-up issue

This should now be fixed.

[tskir]

@joj0s The submission form looks great and the GitHub login works. Unfortunately, when I try to actually do the submission, I get redirected to: http://localhost:8000/traits/github/callback/?code=[some token], which of course doesn't work because I don't have anything deployed on my local machine. It's probably a deployment configuration issue, could you take a look into that when you have time?

In the meanwhile, I'm going to merge this PR because the code changes themselves all look good.