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.

Sign up for GitHub

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

Jump to bottom

[CBO-1417] hearing day pagination #475

Merged
merged 15 commits into from
Feb 25, 2021
Merged

[CBO-1417] hearing day pagination #475

merged 15 commits into from
Feb 25, 2021

Conversation

jsugarman
Copy link
Contributor

@jsugarman jsugarman commented Feb 12, 2021 •
edited
Loading

What

Add next and previous hearing day and hearing pagination

Ticket

CBO-1417 hearing day pagination

Why

Usability testing showed that caseworkers (especially AGFS)
need to view the detail of all hearing days, especially for
trials.

Adding next/previous links made this quicker and easier,
where as previously they could only navigate via the list
of hearings, which was inefficient and frustrating for
longer cases.

How

Unfortunately the usual pagination gems (will_paginate
and kaminari) are not going to work because:

a) we are consuming an API, not Active record models

b) although we are using json_api_client gem, that
follows the jsonapi specification and is supposed
to be compatible with kaminari and will_paginate
the Court data adaptor api itself does not implement the jsonapi pagination specification.

c) The nature of data coming from common platform and then from the adaptor
does not lend itself to "out of the box" pagination solutions and
a custom solution is likely to be more flexible while we progress our
understanding and requirements.

TODO (wip)

  • speed improvement options: attempt to pass a cached prosecution case;
    pass a PageItem set; use caching of api responses
    - have now modified the hearing controller to just query the prosecution case endpoint and retrieve the hearing
    from here, rather than both the pc endpoint and hearing endpoint. I have also spiked caching locally which worked
    but this needs a separate ticket as infrastructure will be needed
  • write issue on CDA for hearing_day filter on hearing endpoint
  • write issue on CDA for jsonapi pagination
  • write issue on CDA for jsonapi sorting

@jsugarman jsugarman changed the title #### What [CBO-1417] hearing day pagination Feb 12, 2021
@jsugarman jsugarman mentioned this pull request Feb 12, 2021
Closed
4 tasks
@jsugarman jsugarman force-pushed the CBO-1417-hearing-day-pagination branch 2 times, most recently from 793602d to 12f145d Compare February 12, 2021 16:23
@jsugarman jsugarman force-pushed the CBO-1417-hearing-day-pagination branch 2 times, most recently from 11ff1f1 to fcfefdb Compare February 12, 2021 17:17
@jsugarman jsugarman force-pushed the CBO-1417-hearing-day-pagination branch from eb132cc to e91d9bf Compare February 18, 2021 18:40
kmahern
kmahern approved these changes Feb 22, 2021
View reviewed changes
Copy link
Contributor

@kmahern kmahern left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@jsugarman jsugarman force-pushed the CBO-1417-hearing-day-pagination branch from a4fa578 to 1871c0b Compare February 22, 2021 15:13
jrmhaig
jrmhaig reviewed Feb 22, 2021
View reviewed changes
Copy link
Contributor

@jrmhaig jrmhaig left a comment

Choose a reason for hiding this comment

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

I've added some comments to the HearingPaginator class but nothing major and I'd not be opposed to merging.

app/helpers/hearing_paginator.rb Outdated Show resolved Hide resolved
app/helpers/hearing_paginator.rb Outdated Show resolved Hide resolved
app/helpers/hearing_paginator.rb Outdated Show resolved Hide resolved
app/helpers/hearing_paginator.rb Outdated Show resolved Hide resolved
app/helpers/hearing_paginator.rb Outdated Show resolved Hide resolved
@jsugarman jsugarman force-pushed the CBO-1417-hearing-day-pagination branch from f658ebf to 89fbd3b Compare February 23, 2021 20:23
@jsugarman
Move brakeman config to file and ignore errors
7feace6 
A parsing error is occuring resulting
from brakeman not being upto date with
ruby 2.7 triple dot `...` argument passing.

Specifically
```
Error: app/helpers/hearing_helper.rb:4 :: parse error on value "..." (tDOT3) Could not parse app/helpers/hearing_helper.rb
```

We are only interested in security warnings
from brakeman so `:exit_on_error: false`
prevents non-zero return codes for parsing
errors.

In addition, since we are now passing three
args/switches to brakeman I have put these
in the default brakeman config file location
so circle will also pick them up.

Haved bumped an existing issue with brakeman
for now.

presidentbeef/brakeman#1483
@jsugarman
Add hearing paginator feature spec
0970ad1
@jsugarman
Add hearing paginator helper
480f006 
Notes:
  1. Disable rubocop Rails/HelperInstanceVariable

  I do not consider this to be relevant in the context
  of a class tha is used to create an object for use in
  views.

  I may raise an issue with rubocop. I note that memoized
  instance variables in helpers do not violate this cop
  due to an issue that was accepted for it.

  2. Move link names to translations

  3. Refactor hearing paginator spec

  The wording of the tests and there use of
  indexes rather than attributes was out of
  date.

 4. Nav links may require a
  ```
  <span class=“govuk-visually-hidden”> some description for screen reader</span>
  ```

 see the [pagination example from moj design system](https://moj-design-system.herokuapp.com/components/pagination/examples/prev-next)

 Need to discuss with the frontend developer
@jsugarman
Use hearing paginator
d4d91d9 
Notes:
  Get hearing spec passing

  just stubbing the extra prosecution case
  query of the controller for now but ideally this
  mechanism will be reversed if i can find a better
  solution to passing the pagination data
  to the hearing controller.
@jsugarman
Display hearing paginator
2096dcd
@jsugarman
Add hearing paginator styling
a98d9ba 
Use @ministryofjustice/frontend/moj css

As requested by designer this it to make use
of the moj [pagination component](https://moj-design-system.herokuapp.com/components/pagination)
which is [defined in the moj-frontend repo](https://github.com/ministryofjustice/moj-frontend/blob/master/package/moj/components/pagination/_pagination.scss)

Notes:
  1. Import moj assets before core typography

  The import order is important should we wish
  to override styles coming from either govuk
  or moj design systems.
@jsugarman
Amend hearing paginator sorting
5ffca6b 
Must match that in the prosecution case
hearing summaries list.

Notes:
  1. Exclude hearing_paginator.rb from Rails/HelperInstanceVariable cop

  This helper is a class that is instantiated
  once per controller instantiation. As such
  it does not make sense to apply the
  Rails/HelperInstanceVariable cop to it at all.

 2. Use @prosecution_case instance var directly

  Since it is not part of, nor needed for the
  public api this can more simply be referred
  to via the instance var itself.
@jsugarman
Decorate prosecution_case in controller
fb50700 
To enable use of the hearing "scope"
for pagination purposes.
@jsugarman
Only query prosecution_case endpoint
85f1156 
Ideally we should be hitting the hearing endpoint only, however, since pagination
currently requires having knowledge of all hearings via the prosecution case endpoint
it is pointless and time-consuming to query both endpoints when the prosecution case
endpoint contains all the info we need.

As a further optimisation we could pass a "pagination collection" instead
and hit the hearing endpoint alone.
@jsugarman
Decorate hearing object in controller
3a2fa97 
Move backend logic out of view.

Also, Remove unrequired locals as these were
from a previous commit and mechanism which is
no longer required.
@jsugarman
Remove attr_writer current_page
8baa7c9 
Not needed by the public interface as
set by instantiation which is performed
during each controller instantiation.
@jsugarman jsugarman force-pushed the CBO-1417-hearing-day-pagination branch from 89fbd3b to 8baa7c9 Compare February 23, 2021 21:00
@jsugarman
Remove cracked/vacted on link
f8d7074 
This link depended on older mechanism
for querying a hearing. It was also a simple
drop in that was not part of designs or user-research

The introduction of pagination (and later sorting)
has broken this simplistic functionality. To get
it working with pagination (and sorting) would have
involved tigh coupling and considerable extra processing
which a) seems excessive for the benefit b) should
be worked as as a separate piece of work if needed.
@jsugarman jsugarman merged commit ecff27c into master Feb 25, 2021
@jsugarman jsugarman deleted the CBO-1417-hearing-day-pagination branch February 25, 2021 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kmahern kmahern kmahern approved these changes

@jrmhaig jrmhaig jrmhaig approved these changes

@naseberry naseberry Awaiting requested review from naseberry

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsugarman @jrmhaig @naseberry @kmahern