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

refactor: expose court report methods #5590

Merged
merged 1 commit into from
Apr 14, 2024

Conversation

elasticspoon
Copy link
Collaborator

@elasticspoon elasticspoon commented Mar 27, 2024

Context

I will soon need to make a few changes to report generation and the current CaseCourtReportContext makes it very difficult to change behavior.

The Problem

Currently CaseCourtReportContext has a single public method that outputs a very complex object:

  {
    created_date: I18n.l(Time.current.in_time_zone(@time_zone).to_date, format: :full, default: nil),
    casa_case: prepare_case_details,
    case_contacts: prepare_case_contacts,
    case_court_orders: prepare_case_orders,
    case_mandates: prepare_case_orders, # backwards compatible with old Montgomery template - keep this! TODO test full generation
    latest_hearing_date: latest_hearing_date.nil? ? "___<LATEST HEARING DATE>____" : I18n.l(latest_hearing_date.date, format: :full, default: nil),
    org_address: org_address(is_default_template),
    volunteer: volunteer_info,
    hearing_type_name: @court_date&.hearing_type&.name || "None"
  }

The hash itself gets built using smaller methods but is tested by creating the full hash then inspecting each key. Ex:

expect(court_report_context.context[:casa_case][:court_date]).to eq("March 1, 2021")

This makes it hard to extend the behavior of the class and write additional tests.

What changed, and why?

This PR reworks some of the methods and tests for CaseCourtReportContext.

  • Expose methods in court report context to make unit testing easier.
  • Write additional unit tests.
  • Remove old tests.
  • Remove redundant tests, some behavior is already tested by CaseContactsContactDates.

How will this affect user permissions?

None

How is this tested? (please write tests!) 💖💪

Unit tests

Expose methods in court report context to make unit
testing easier.

Write additional unit tests.
Remove old tests.

Remove redundant tests, some behavior is
already tested by the Dates object.
@github-actions github-actions bot added ruby Pull requests that update Ruby code Tests! 🎉💖👏 labels Mar 27, 2024
Comment on lines +119 to +125
# not sure how to test this without just restating the code
# context 'when there are interviewees' do
# it 'it calls CaseContactsContactDates with filtered values' do
# create_list(:case_contact_contact_type, 3, case_contact: create(:case_contact, casa_case: casa_case))
# context.case_contacts
# end
# end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how best to test the case where there are interviewees, should I even test it?

  def case_contacts
    cccts = CaseContactContactType.includes(:case_contact, :contact_type).where("case_contacts.casa_case_id": @casa_case.id)
    interviewees = filter_out_old_case_contacts(cccts)
    return [] unless interviewees.size.positive?

    CaseContactsContactDates.new(interviewees).contact_dates_details
  end

All this could really test is that the ActiveRecord query is correct, and its not a very complex query. Is there value in it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am somewhat unclear on what an interviewee is
In general I am in favor of more testing rather than less tho

@elasticspoon elasticspoon marked this pull request as ready for review March 27, 2024 21:32
Copy link
Collaborator

@compwron compwron left a comment

Choose a reason for hiding this comment

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

In general I am in favor of this but I am nervous about it possibly breaking court report generation somehow

@compwron compwron merged commit cf332dc into rubyforgood:main Apr 14, 2024
11 of 12 checks passed
@elasticspoon
Copy link
Collaborator Author

In general I am in favor of this but I am nervous about it possibly breaking court report generation somehow

It certainly could be just because the testing around report generation is pretty convoluted. That said these changes still pass all the tests directly related to report generation.

In my other PR I test the report like this:

context "#generate_to_string" do

Which imo makes it much easier to be sure we get all the edge cases. Ideally I would like to refactor the whole report test to be of that form. But it's a pretty big shift so idk.

@elasticspoon elasticspoon deleted the refactor-context branch April 14, 2024 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ruby Pull requests that update Ruby code Tests! 🎉💖👏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants