[Don't merge yet] Integrate Zenodo API for Plan PDF uploads #2132
Conversation
Bodacious
changed the title
There was a problem hiding this comment.
Looks good overall @Bodacious. Left a comment about one of the user updates.
I'm noticing a lot of use of ENV variables. This approach makes assumptions about how people deploy and configure their installations. It would be preferable to assign these values to the Rails.configuration somewhere on initialization perhaps rather than referencing them directly in the application code. Something like:
Rails.application.configure do
config.oauth2['zenodo']['client_id'] = ENV["ZENODO_CLIENT_ID"] || config.branding['oauth2']['zenodo']['client_id']
endAnd then in the code:
OAuth2::Client.new(
Rails.config.oauth2[provider]['client_id'],
Rails.config.oauth2[provider]['client_secret'],
site: provider.classify.constantize.const_get("BASE"),
logger: Logger.new(Rails.root.join("log", "#{provider}.log"), 'weekly')
)| @@ -39,6 +39,18 @@ def plan_attribution(attribution) | |||
| "<strong>#{prefix}</strong> #{attribution.join(', ')}" | |||
| end | |||
|
|
|||
| def download_plan_page_title(plan, phase, hash) | |||
There was a problem hiding this comment.
thanks for moving these over. makes more sense for them to be in the exports_helper
| client = client_for_oauth2_provider(params[:provider]) | ||
| @token = client.auth_code.get_token(params[:code], | ||
| redirect_uri: REDIRECT_URI % params) | ||
| current_user.update!(zenodo_access_token: @token.token) |
There was a problem hiding this comment.
zenodo_access_token is very specific while the rest of the class is generic. Maybe something like current_user.update!("#{provider}_access_token": @token.token) instead
| @@ -0,0 +1,2 @@ | |||
| <h1>Arpha::Plans#show</h1> | |||
| <p>Find me in app/views/arpha/plans/show.html.erb</p> | |||
There was a problem hiding this comment.
? Not sure what this is doing
NOTE: `access_token` param has been hidden from logger. This isn't strictly necessary, since that param isn't currently being logged in production mode. But I've kept it that way to offer a bit more security in case someone changes the logger level later.
Removed this for debugging and forgot to reinstate it. Thanks Brakeman!
|
Ignore my prior comments with regard to the ENV variables. @xsrust introduced me to |
|
@xsrust @briri just adding comment here that this is the work we started on Zenodo, we discussed with @mariapraetzellis and @pherterich yesterday as we planned this for autumn 2020 in our Roadmap |
|
@martaribeiro & @gjacob24 I guess we close this and keepthe branch as suggested by @briri |
Changes proposed in this PR:
Please note: to test this, you'll need to do the following:
HOSTenv variable (for OAuth 2.0 redirects). This should be set to whatever master domain the app is running on (will be 'localhost:3000' in development).CLIENT_IDandCLIENT_SECRETfor the applications, and store these as env variables.Go to the "Share" tab for a given Plan and click the Zenodo button at the foot of the page