Add appeals to BOPS endpoints in PostsubmissionSchema format (DSNPI-1008)

[apricot13]

Hey!

We will slowly be submitting PR's to convert the public endpoints to the ODP schema for the DPR, this one is for the appeals section.

Description of change

This PR makes a change to the v2 public search and v2 public show endpoints to return appeals in the structure we've put in the PostsubmissionApplication schema and the DPR.

Decisions

• I have purposely put files in engines/bops_api/app/views/bops_api/v2/shared/ and not public as the intention is in the future to add a new PublishedSubmission schema which is a subset of the Postsubmission and this would require moving and renaming all sorts of files down the line when currently everything we're doing applies at this top level.
• I have created a new helper PostsubmissionApplicationSchemaHelper as a starting point for Post submission specific formatting
• I have placed the appeals data inside of a data object in the returned json all on its lonesome for now so that we can keep adding to it as we migrate to the new schema.

Known issues

I haven't updated the swagger_doc.yml as I was having trouble validating it in the swagger editor - hoping to get some tips on this from this PR!

I have also kept the tests fairly minimal, I have some example's documented in the ODP schema repo which might be good to build tests against as we build up the new schema but I felt adding all that was a little beyond the scope of this PR - something to discuss in our next call?

[benjamineskola]

For what it's worth, we usually convert datetimes to YYYY-MM-DD format with foo.to_date.to_s.

(Normally we'd display dates as YYYY-MM-DD and datetimes as full ISO8601, but we might still have a few datetimes that should probably be stored as dates and so we truncate those — and of course if there's an external requirement for a particular format.)

I'd be really suspicious if we need to display a date in full ISO8601 format though, I wouldn't want to make it look like we have more accuracy in the data than we actually do. (But I don't think the format_postsubmission_datetime method is actually being used?)

[apricot13]

Ah ok so would you suggest I switch to using foo.to_date.to_s in this helper? Does it ensure that the returned result is utc?

I still need to do a final walkthrough of the date/datetime setup for the postSubmission - for now I've followed the same setup as we had before but I still feel for some aspects a full date time would be useful for context and auditing even if its never shown in the back/frontend.

(and yeh format_postsubmission_datetime was just me adding in both types of dates the postsubmission supports its not actually used yet!)

[benjamineskola]

Yeah, there are definitely places where we store a datetime even if in practice we never actually show it: I think it's good for recording when an event actually occurred specifically. (So I think it normally ends up being datetimes for the past, dates for the future: like deadlines and so on.)

My only concern about using a datetime when we store only a date is that it might be misleading, for cases like deadlines, to include 00:00:00 when in practice it's probably actually going to be the start or end of the working day or something like that. But obviously there might be other situations I haven't thought of where it does make sense to do so.

[apricot13]

I don't think theres currently any data thats being stored in BOPS as a date expecting to be returned as a dateTime in the new schema - though it may be in the future that we end up having to update some to be dateTimes in BOPS just so we're not returning empty times.

[benjamineskola]

I think you can leave the variable name out here and the variable with the same name as the key will be used:

Suggested change

	json.partial! "bops_api/v2/public/shared/show", planning_application: planning_application
	json.partial! "bops_api/v2/shared/postsubmissionApplication/postsubmissionApplication", planning_application: planning_application
	json.partial! "bops_api/v2/public/shared/show", planning_application:
	json.partial! "bops_api/v2/shared/postsubmissionApplication/postsubmissionApplication", planning_application:

[apricot13]

I've made that change! its so weird because it gave me no end of arguments the other day when I left it out and now its behaving! 🤷‍♀️

[apricot13]

OK now the error has come back! any idea what would be causing this? I've put the code back to how it was for now to be able to use the endpoint (I've rebuild and rerun the container and it still errors)

json.partial! "bops_api/v2/shared/metadata"

json.data @planning_applications do |planning_application|
  json.partial! "bops_api/v2/public/shared/show", planning_application: 
  json.partial! "bops_api/v2/shared/postsubmissionApplication/postsubmissionApplication", planning_application:   
end

This error only shows when engines/bops_api/app/views/bops_api/v2/public/search.json.jbuilder is :

# frozen_string_literal: true

json.partial! "bops_api/v2/shared/metadata"

json.data @planning_applications do |planning_application|
  json.partial! "bops_api/v2/public/shared/show", planning_application:
  json.partial! "bops_api/v2/shared/postsubmissionApplication/postsubmissionApplication", planning_application:
end

[benjamineskola]

What errors are you getting from this? It appears to work fine when I change it but I might be missing something.

[apricot13]

ooh maybe its that file loading issue @pixeltrix and I were talking about yesterday! (or possibly the rubocop formatting I'm using vscode!)

[benjamineskola]

These tests all fail because they refer to methods that don't exist: should it be format_postsubmission_date and format_postsubmission_datetime instead?

[apricot13]

I never thought I'd miss husky running the tests every five minutes! Fixed now hopefully!

[benjamineskola]

There are a few places where Rubocop is showing up formatting errors: mainly whitespace/newlines and quote marks — could you run rubocop --autocorrect and update the PR with the changes, please?

[benjamineskola]

We've found in the past that it's easier to define a schema for things if we have values that are always present, but potentially nil, rather than being omitted if they're nil.

So in this case we'd probably leave off the if-statements in this file and just let it return the nils.

But you're the API consumer (and probably more familiar with jsonschema than me) so would be open to your perspective on that!

[apricot13]

Yeh, I see what you mean. I've mostly followed the convention from the PrototypeApplication and since the schema is generated from TypeScript, the general recommendation is to use field?: string rather than field: string | null. It's also much easier to work with field?: string when dealing directly with the types. 😅. It may also be, and I'd need to check this, that the library that converts TypeScript to Json-Schema (ts-json-schema-generator) is opinionated on this?

In the examples I have purposely left out fields that are not relevant or applicable to the application - Appeal determined application vs Council determined application. Data not being present in a result usually means that is is optional and/or not applicable? which is definitely true in the case of appeal data though it may not be the case for some of the other fields later on! If you can no longer submit an appeal after 6 months then appeal: null being present would seem odd and could imply you can still appeal?

[benjamineskola]

Ah, Andrew has suggested that this could be

Suggested change

	date&.utc&.strftime("%Y-%m-%d")
	date&.utc&.to_date.iso8601

Which I think might make the intent even clearer?

(I'm actually not sure if utc is even needed when we're only dealing with dates, since the DST change should never change the date — it'd be different if we had to deal with times outside the UK.)

[apricot13]

I think I've been getting ahead of myself here tbh, I've changed the functionality and description of this helper to reflect its role as go between for dates stored as datetimes that should be dates until they get converted some day!

Theres an issue here to standardise the dates as UTC in the API endpoints and use the at/date naming convention too. Even though this standard is currently only used in the UK, it's pretty standard to use UTC in APIs for consistency. We currently get all sorts of different date formats back from BOPS/PlanX so it would be good to agree on one format.