Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generalizing JOSS #361

Merged
merged 30 commits into from
Dec 22, 2017
Merged

Generalizing JOSS #361

merged 30 commits into from
Dec 22, 2017

Conversation

arfon
Copy link
Member

@arfon arfon commented Dec 1, 2017

👋 @parrish. I've moved this PR over to a branch on the parent repository so I can push updates to it too. Copying in your PR body from #353 here:


This looks like a larger PR than it really is since there's a fair bit of template reworking. It's probably fairly digestable per-commit, though I'm happy to split this out into multiple PR's if it's easier.

Motivated by #329, this PR

  • Loads the editor team from GitHub for the reviewer assignment and suggested reviewer on the submission.
  • Generalizes the templates to use a settings.yml file to specify the easily extractable language and configuration
  • Extracts as much customizable content from templates and template structure as possible. These partials now live in app/views/content
  • Adds a simple editors admin management UI at http://staging.joss.theoj.org/editors
    • Can be bootstrapped via the db/seeds
    • Currently, it's independent from the GitHub editor team, but that might be a cool feature.
    • It requires the admin flag on a user account, just ping me if you can't access it
  • Adds a configurable list of paper submission types.

Along with openjournals/whedon-api#11, this should get us into a pretty good position to have multiple journals running. 🎉

In-progress deploys are going to http://staging.joss.theoj.org

Feedback welcome!

<%- if paper.kind.present? %>
<%= render partial: "content/github/#{paper.kind}_review_checklist", locals: local_assigns %>
<%- else %>
<%= render partial: "content/github/review_checklist", locals: local_assigns %>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does local_assigns make the paper variable available to the partial at content/github/review_checklist @parrish?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's basically a passthrough for the locals that used to render shared/review_body

STR
)

Editor.create(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these all be kind: topic?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's the column default, but there's no harm in being explicit here.

<%= f.text_field :last_name %>
</div>
<div class="field">
<%= f.label :login %><br>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this should be a select based on Repository.editors

@arfon
Copy link
Member Author

arfon commented Dec 1, 2017

Oops. I just realized that you also merged your changes from #353 into #330.

@labarba
Copy link
Member

labarba commented Dec 5, 2017

Hi @arfon — when do you estimate that this PR might be merged in?
We're looking forward to kick starting JOSE!
//cc. @kyleniemeyer

@arfon arfon temporarily deployed to joss-staging December 6, 2017 00:58 Inactive
@arfon arfon temporarily deployed to joss-staging December 6, 2017 01:25 Inactive
@arfon arfon temporarily deployed to joss-staging December 6, 2017 01:38 Inactive
@arfon arfon temporarily deployed to joss-staging December 6, 2017 02:14 Inactive
@arfon arfon temporarily deployed to joss-staging December 6, 2017 02:15 Inactive
@arfon arfon mentioned this pull request Dec 6, 2017
@arfon arfon temporarily deployed to joss-staging December 6, 2017 16:08 Inactive
@arfon arfon temporarily deployed to joss-staging December 6, 2017 16:12 Inactive
@arfon arfon temporarily deployed to joss-staging December 6, 2017 16:42 Inactive
@@ -1,6 +1,6 @@
class SessionsController < ApplicationController
def create
user = User.from_omniauth(env["omniauth.auth"])
user = User.from_omniauth(ENV["omniauth.auth"])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦‍♂️

@arfon
Copy link
Member Author

arfon commented Dec 7, 2017

Hi @arfon — when do you estimate that this PR might be merged in?
We're looking forward to kick starting JOSE!

👋 @labarba @kyleniemeyer - we're getting really close. I've been testing all of these changes over the last couple of days and things are looking pretty solid. I hope to finish up this testing this week.

@labarba
Copy link
Member

labarba commented Dec 16, 2017

Are we there yet?

db/seeds.rb Outdated
last_name: "Githinji",
login: "biorelated",
email: "",
avatar_url: "http://joss.theoj.org/assets/editors/george-d24f98577bcc40a745b81f7ee5c4b6a3e5f150b563f940cb792ba92f88ed6483.jpg",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parrish - what should we do about URLs like this? How does encoding the asset pipeline stuff (i.e. george-d24f98577bcc40a745b81f7ee5c4b6a3e5f150b563f940cb792ba92f88ed6483.png work in the long run?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess perhaps it doesn't matter as long as the file doesn't change? http://guides.rubyonrails.org/asset_pipeline.html#what-is-fingerprinting-and-why-should-i-care-questionmark

@arfon arfon temporarily deployed to joss-staging December 21, 2017 01:08 Inactive
@arfon arfon temporarily deployed to joss-staging December 21, 2017 01:13 Inactive
@arfon
Copy link
Member Author

arfon commented Dec 21, 2017

Are we there yet?

I think so yes. I'm planning on migrating the JOSS site over to this updated code later today and once that's done I think we're ready to start rolling out the new JOSE codebase.

@arfon arfon temporarily deployed to joss-staging December 22, 2017 01:03 Inactive
@arfon arfon temporarily deployed to joss-staging December 22, 2017 01:08 Inactive
@arfon arfon temporarily deployed to joss-production December 22, 2017 01:16 Inactive
@arfon arfon merged commit 52560f9 into master Dec 22, 2017
@arfon arfon mentioned this pull request Dec 22, 2017
@arfon arfon deleted the generalize branch February 5, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants