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

Display stalled statements list in financial console #2127

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

slawosz
Copy link
Contributor

@slawosz slawosz commented Jan 20, 2025

Added:

  • list of stalled statements
  • better message if statement is stalled
    image

Context

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

Copy link
Contributor

Copy link
Contributor

@jebw jebw left a comment

Choose a reason for hiding this comment

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

Looks great, really nice addition 🙌 I've made a couple of minor points around the scope in the controller inline.

Another request is to add a request spec for the controller - it can be pretty minimal, just verify 200 response for authorised and 401 for unauthorised but it does need to a request spec to be checking the authorization

https://github.com/DFE-Digital/npq-registration/blob/main/spec/requests/npq_separation/admin/schedules_controller_spec.rb#L124-L139

@@ -1,4 +1,6 @@
class Statement < ApplicationRecord
AUTHORISATION_GRACE_TIME = 15 * 60
Copy link
Contributor

Choose a reason for hiding this comment

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

This can use rails' period utilities, ie 15.minutes

@@ -41,6 +43,7 @@ class Statement < ApplicationRecord
scope :unpaid, -> { with_state(%w[open payable]) }
scope :paid, -> { with_state("paid") }
scope :next_output_fee_statements, -> { with_state("open").with_output_fee.order(:deadline_date).where("deadline_date >= ?", Date.current) }
scope :with_delayed_authorisations, -> { with_state("payable").where("marked_as_paid_at < ?", AUTHORISATION_GRACE_TIME.seconds.ago) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This would then change to AUTHORISATION_GRACE_TIME.ago

@@ -0,0 +1,5 @@
class NpqSeparation::Admin::Finance::Statements::StaleController < NpqSeparation::AdminController
def index
@statements = Statement.with_delayed_authorisations
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to use Statements::Query like PaidController does

Also I'm maybe missing something but you've got the pagination helper in the view but nothing seems to be passing the page into the above scope?

Copy link
Contributor

@jebw jebw left a comment

Choose a reason for hiding this comment

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

Looks great, but linter and some of the specs are failing?

Copy link
Contributor

@jebw jebw left a comment

Choose a reason for hiding this comment

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

LGTM 🙌

@slawosz slawosz added this pull request to the merge queue Jan 28, 2025
github-merge-queue bot pushed a commit that referenced this pull request Jan 28, 2025
* Display stalled statements list in financial console and proper notification in statement view page

* Refactor stalled statements display: specs and code
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
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.

2 participants