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

Adds review workflow. #493

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ gem 'frozen_record' # For licenses
gem 'honeybadger'
gem 'kaminari' # For pagination
gem 'okcomputer'
gem 'state_machines-activerecord'
gem 'view_component'
gem 'whenever', require: false

Expand Down
8 changes: 8 additions & 0 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,13 @@ GEM
net-sftp (>= 2.1.2)
net-ssh (>= 2.8.0)
ostruct
state_machines (0.6.0)
state_machines-activemodel (0.9.0)
activemodel (>= 6.0)
state_machines (>= 0.6.0)
state_machines-activerecord (0.9.0)
activerecord (>= 6.0)
state_machines-activemodel (>= 0.9.0)
stimulus-rails (1.3.4)
railties (>= 6.0.0)
stringio (3.1.2)
Expand Down Expand Up @@ -608,6 +615,7 @@ DEPENDENCIES
solid_cable
solid_cache
solid_queue
state_machines-activerecord
stimulus-rails
turbo-rails
tzinfo-data
Expand Down
5 changes: 5 additions & 0 deletions app/assets/stylesheets/application.bootstrap.scss
Original file line number Diff line number Diff line change
Expand Up @@ -161,4 +161,9 @@ table.table-treegrid {

textarea {
resize: both;
}

// This is a custom alert variant that is used for the review alert.
.alert.alert-input {
background-color: var(--stanford-10-black);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<%= render Elements::Tables::TableComponent.new(
id:, label: 'Drafts', show_label: false
) do |component| %>
<% component.with_header(
headers: ['Deposit', 'Collection', 'Owner', 'Last modified']
) %>
<% works.each do |work| %>
<% component.with_row(values: values_for(work), id: id_for(work)) %>
<% end %>
<% end %>
40 changes: 40 additions & 0 deletions app/components/dashboard/show/pending_review_list_component.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# frozen_string_literal: true

module Dashboard
module Show
# Component for rendering a table on the work show page with works pending review.
class PendingReviewListComponent < ApplicationComponent
def initialize(works:)
@works = works
super()
end

attr_reader :works

def id
'pending-review-table'
end

def id_for(work)
dom_id(work, id)
end

def values_for(work)
[
link_to(work.title, link_for(work)),
link_to(work.collection.title, collection_path(druid: work.collection.druid)),
work.user.name,
I18n.l(work.updated_at, format: '%b %d, %Y')
]
end

private

def link_for(work)
return wait_works_path(work) unless work.druid

work_path(druid: work.druid)
end
end
end
end
4 changes: 3 additions & 1 deletion app/components/elements/alert_component.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
<%= tag.div role: 'alert', class: classes, data: do %>
<div class="fs-3 me-3 align-self-center d-flex justify-content-center"><%= icon %></div>
<% if icon? %>
<div class="fs-3 me-3 align-self-center d-flex justify-content-center"><%= icon %></div>
<% end %>
<div class="text-body">
<% if title.present? %>
<div class="fw-semibold"><%= title %></div>
Expand Down
9 changes: 7 additions & 2 deletions app/components/elements/alert_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
module Elements
# Component for rendering an alert.
class AlertComponent < ApplicationComponent
# Variants are :danger, :success, :note, :info, :warning
# Variants are :danger, :success, :note, :info, :warning, :input
# input is not part of the component library
def initialize(title: nil, variant: :info, dismissible: false, value: nil, data: {}, classes: []) # rubocop:disable Metrics/ParameterLists
raise ArgumentError, 'Invalid variant' unless %i[danger success note info warning].include?(variant.to_sym)
raise ArgumentError, 'Invalid variant' unless %i[danger success note info warning input].include?(variant.to_sym)

@title = title
@variant = variant
Expand Down Expand Up @@ -34,6 +35,10 @@ def dismissible?
@dismissible
end

def icon?
helpers.respond_to?(:"#{variant}_icon")
end

def icon
helpers.public_send(:"#{variant}_icon")
end
Expand Down
30 changes: 30 additions & 0 deletions app/components/works/edit/submit_component.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# frozen_string_literal: true

module Works
module Edit
# Component for rendering a submit button
class SubmitComponent < ApplicationComponent
def initialize(form_id:, work:, collection:, classes: [])
@form_id = form_id
@work = work
@collection = collection
@classes = classes
super()
end

attr_reader :form_id, :work, :collection, :classes

def call
render Elements::Forms::SubmitComponent.new(form_id:, label:, classes:)
end

def label
if collection.review_enabled? && (work.nil? || !helpers.allowed_to?(:review?, work))
'Submit for review'
else
'Deposit'
end
end
end
end
end
16 changes: 16 additions & 0 deletions app/components/works/show/review_component.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<%= render Elements::AlertComponent.new(variant: :input) do %>
<div class="h3">Review all details below then approve this deposit or return with comments.</div>
<%= form_with model: review_form, url: review_work_path, method: :put do |form| %>
<div class="form-check mb-3">
<%= form.radio_button :review_option, 'approve', class: 'form-check-input' %>
<%= form.label :review_option_approve, 'Approve', class: 'form-check-label' %>
</div>
<div class="form-check mb-3">
<%= form.radio_button :review_option, 'reject', class: 'form-check-input' %>
<%= form.label :review_option_reject, 'Return with comments', class: 'form-check-label' %>
<%= render Elements::Forms::TextareaFieldComponent.new(form:, field_name: :reject_reason, label: 'Reason for returning', hidden_label: true) %>
</div>

<%= render Elements::Forms::SubmitComponent.new(label: 'Submit') %>
<% end %>
<% end %>
20 changes: 20 additions & 0 deletions app/components/works/show/review_component.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# frozen_string_literal: true

module Works
module Show
# Component providing a form for submitting a review.
class ReviewComponent < ApplicationComponent
def initialize(work:, review_form:)
@work = work
@review_form = review_form
super()
end

attr_reader :review_form

def render?
@work.pending_review? && helpers.allowed_to?(:review?, @work)
end
end
end
end
10 changes: 10 additions & 0 deletions app/components/works/show/review_rejected_component.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<%= render Elements::AlertComponent.new(variant: :danger) do %>
<p>
The reviewer for this collection has returned the deposit for the following reason(s).

Please fix the items given below and submit your deposit again for review.
</p>
<blockquote class="blockquote">
<p><%= review_rejected_reason %></p>
</blockquote>
<% end %>
19 changes: 19 additions & 0 deletions app/components/works/show/review_rejected_component.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# frozen_string_literal: true

module Works
module Show
# Component for rendering an alert for a rejected review.
class ReviewRejectedComponent < ApplicationComponent
def initialize(work:)
@work = work
super()
end

delegate :review_rejected_reason, to: :@work

def render?
@work.rejected_review?
end
end
end
end
6 changes: 5 additions & 1 deletion app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,13 @@ def deposit?
params[:commit] == 'Deposit'
end

def request_review?
params[:commit] == 'Submit for review'
end

# NOTE: a `nil` validation context runs all validations without an explicit context
def validation_context
return :deposit if deposit?
return :deposit if deposit? || request_review?

nil
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/dashboard_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ def show
private

def druids_for_works_for_current_user
current_user.works.where.not(druid: nil).pluck(:druid)
current_user.your_works.where.not(druid: nil).pluck(:druid)
end
end
67 changes: 53 additions & 14 deletions app/controllers/works_controller.rb
Original file line number Diff line number Diff line change
@@ -1,20 +1,22 @@
# frozen_string_literal: true

# Controller for a Work
class WorksController < ApplicationController
before_action :set_work, only: %i[show edit update destroy]
class WorksController < ApplicationController # rubocop:disable Metrics/ClassLength
before_action :set_work, only: %i[show edit update destroy review]
before_action :check_deposit_job_started, only: %i[show edit]
before_action :set_work_form_from_cocina, only: %i[show edit]
before_action :set_content, only: %i[show edit]
before_action :set_status, only: %i[show edit destroy]
before_action :set_presenter, only: %i[show edit]
before_action :set_work_form_from_cocina, only: %i[show edit review]
before_action :set_content, only: %i[show edit review]
before_action :set_status, only: %i[show edit destroy review]
before_action :set_presenter, only: %i[show edit review]

def show
authorize! @work

# This updates the Work with the latest metadata from the Cocina object.
# Does not update the Work's collection if the collection cannot be found.
ModelSync::Work.call(work: @work, cocina_object: @cocina_object, raise: false)

@review_form = ReviewForm.new
end

def new
Expand Down Expand Up @@ -52,11 +54,9 @@ def create # rubocop:disable Metrics/AbcSize

# The validation_context param determines whether extra validations are applied, e.g., for deposits.
if @work_form.valid?(validation_context)
# Setting the deposit_job_started_at to the current time to indicate that the deposit job has started and user
# should be "waiting".
work = Work.create!(title: @work_form.title, user: current_user, deposit_job_started_at: Time.zone.now,
collection: @collection)
DepositWorkJob.perform_later(work:, work_form: @work_form, deposit: deposit?)
perform_deposit(work:)
redirect_to wait_works_path(work.id)
else
@content = Content.find(@work_form.content_id)
Expand All @@ -65,16 +65,13 @@ def create # rubocop:disable Metrics/AbcSize
end
end

def update # rubocop:disable Metrics/AbcSize
def update
authorize! @work

@work_form = WorkForm.new(**update_work_params)
# The validation_context param determines whether extra validations are applied, e.g., for deposits.
if @work_form.valid?(validation_context)
# Setting the deposit_job_started_at to the current time to indicate that the deposit job has started and user
# should be "waiting".
@work.update!(deposit_job_started_at: Time.zone.now)
DepositWorkJob.perform_later(work: @work, work_form: @work_form, deposit: deposit?)
perform_deposit(work: @work)
redirect_to wait_works_path(@work.id)
else
@content = Content.find(@work_form.content_id)
Expand Down Expand Up @@ -106,12 +103,28 @@ def wait
redirect_to work_path(druid: work.druid) if work.deposit_job_finished?
end

def review
authorize! @work

@review_form = ReviewForm.new(**review_form_params)
if @review_form.valid?
redirect_path = perform_review
redirect_to redirect_path
else
render :show, status: :unprocessable_entity
end
end

private

def work_params
params.expect(work: WorkForm.user_editable_attributes + [WorkForm.nested_attributes])
end

def review_form_params
params.expect(review: %i[review_option reject_reason])
end

def update_work_params
work_params.merge(druid: params[:druid])
end
Expand Down Expand Up @@ -168,4 +181,30 @@ def new_work_form
release_date: @collection.max_release_date
)
end

def perform_deposit(work:)
# Setting the deposit_job_started_at to the current time to indicate that the deposit job has started and user
# should be "waiting".
work.update!(deposit_job_started_at: Time.zone.now)
deposit = deposit?
if request_review?
work.request_review!
deposit = false # Will be saved, but not deposited until approved.
end
DepositWorkJob.perform_later(work:, work_form: @work_form, deposit:)
end

# @return [String] path to redirect to after review
def perform_review
if @review_form.review_option == 'approve'
# Deposit
@work.update!(deposit_job_started_at: Time.zone.now)
@work.approve!
DepositWorkJob.perform_later(work: @work, work_form: @work_form, deposit: true)
wait_works_path(@work.id)
else
@work.reject_with_reason!(reason: @review_form.reject_reason)
work_path(druid: @work.druid)
end
end
end
10 changes: 10 additions & 0 deletions app/forms/review_form.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# frozen_string_literal: true

# Form object for reviewing a work.
class ReviewForm < ApplicationForm
attribute :review_option, :string, default: 'approve'
validates :review_option, inclusion: { in: %w[approve reject] }

attribute :reject_reason, :string
validates :reject_reason, presence: true, if: -> { review_option == 'reject' }
end
4 changes: 4 additions & 0 deletions app/models/collection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,8 @@ def max_release_date
def add_user_as_manager
managers << user if user.present? && managers.exclude?(user)
end

def review_enabled?
review_enabled
end
end
4 changes: 4 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,8 @@ def your_collections
def your_works
Work.where(collection: your_collections)
end

def your_pending_review_works
Work.where(collection: reviewer_for).with_review_state(:pending_review)
end
end
Loading