[CPDNPQ-2242] admin bulk reject applications

[alkesh]

Context

Ticket: https://dfedigital.atlassian.net/browse/CPDNPQ-2242

Bulk operations:

• reject applications
• change applications to pending

Changes proposed in this pull request

production ready - when compared to the previous spike (#2046):

• added bulk reject applications
• improved UI
• background jobs
• specs

I tried the jobs locally, using a file that contained all the applications I had locally - which was 1459 applications - the background job took:

• reject applications - 2 seconds
• change to pending - 12 seconds

[alkesh]

moved to app/services/bulk_operation/bulk_change_applications_to_pending.rb

[github-actions]

Review app deployed to https://npq-registration-review-2088-web.test.teacherservices.cloud/

[rwrrll]

This will be a big win when it goes live 👏 left a few comments, mostly just suggestions really!

[rwrrll]

I can't see anything else inheriting from this, is it just for namespacing? If so, does it need to inherit / should it be a module?

[alkesh]

It's definitely a required controller - used for the bulk operations index page, which just has the two links for "Revert applications to pending" and "Reject applications".
It needs to inherit from NpqSeparation::AdminController, just so that it's only accessible by admins.

[rwrrll]

Do we want a find_by! so that it blows up if the application doesn't exist, or is nil fine?

[alkesh]

nil is fine here - the Applications::Reject has a presence validator for the application,
and the resulting outcome will be a "Not found" - so there is an entry in the result for every ID passed.

I did notice there's a spec missing for this though - will add.

[slawosz]

Looks good, several comments regarding potential code cleanup.

[slawosz]

Both controllers, RejectApplicationsController and RevertApplicationsToPendingController looks very similar. I wonder if it would be possible to extract a parent controller like

module NpqSeparation::Admin::BulkOperations
  class BaseController < NpqSeparation::AdminController
    before_action :set_bulk_operations, :set_bulk_operation, only: %i[index create]
    before_action :find_bulk_operation, only: %i[run show]

    def create
      if (file = params.dig(:bulk_operation_reject_applications, :file))
        bulk_operation_scope.not_started.destroy_all
        @bulk_operation.file.attach(file)
        if @bulk_operation.valid?
          @bulk_operation.save!
          @bulk_operation.update!(row_count: @bulk_operation.file.download.lines.count)
          return redirect_to redirect_to_path
        end
      end

      render :index, status: :unprocessable_entity
    end

    def run
      @bulk_operation.update!(started_at: Time.zone.now, ran_by_admin_id: current_admin.id)
      job_class.perform_later(bulk_operation_id: @bulk_operation.id)
      redirect_to redirect_to_path
    end

    def show
      # empty method, because rubocop will complain in the before_action otherwise
    end

  private

    def set_bulk_operations
      @bulk_operations = bulk_operation_scope.all.includes([file_attachment: :blob]).order(:created_at)
    end

    def set_bulk_operation
      @bulk_operation = bulk_operation_scope.new admin: current_admin
    end

    def find_bulk_operation
      @bulk_operation = bulk_operation_scope.find(params[:id])
    end
  end
end

and

module NpqSeparation::Admin::BulkOperations
  class RejectApplicationsController < BaseController
    private
    def redirect_to_path
      :npq_separation_admin_bulk_operations_reject_applications
    end

    def bulk_operation_scope
      BulkOperation::RejectApplications
    end

    def job_class
      BulkOperation::BulkRejectApplicationsJob
    end
  end
end

[alkesh]

I think my view on this is that it is premature optimisation. I'm a fan of the rule of three.
If the next time we need a bulk operation, and that operation is as similar as the current 2 - then we extract a parent controller at that point.

[slawosz]

Similar to the controllers comment - both workers are very similar and maybe could be extracted to one?
I guess dry_run argument could be part of the interface.

[slawosz]

Again, both services have very similar code. I wonder if it makes sense to extract them? One good argument for the extraction would be unification - if the future bulk service would not fit into intended structure, it could mean that it would be too complex and simplification would be recommended?

[slawosz]

There would be possible to use template inheritance as well.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud