Skip to content
This repository has been archived by the owner on Aug 16, 2024. It is now read-only.

feat: Filter out unused irfo permit types from dropdown #190

Closed
wants to merge 1 commit into from
Closed

feat: Filter out unused irfo permit types from dropdown #190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion module/Olcs/src/Service/Data/IrfoGvPermitType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
*/
class IrfoGvPermitType extends AbstractDataService implements ListDataInterface
{
public const UNUSED_TYPE_IDS = [5, 8, 12, 20, 15, 19, 16];

/**
* Format data
*
Expand Down Expand Up @@ -70,10 +72,22 @@ public function fetchListData()
$this->setData('IrfoGvPermitType', false);

if (isset($response->getResult()['results'])) {
$this->setData('IrfoGvPermitType', $response->getResult()['results']);
$this->setData('IrfoGvPermitType', $this->filterArrayById($response->getResult()['results']));
}
}

return $this->getData('IrfoGvPermitType');
}

/**
* Filter unused IDs out of the array for display (cant remove from DB as 100s of permit records still reference the ID, but not needed for new permits)
*
* @param array $data Data
*
* @return array
*/
public function filterArrayById(array $data): array
{
return array_filter($data, fn($record) => !in_array($record['id'], self::UNUSED_TYPE_IDS));
}
Comment on lines +82 to +92
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this functionality be more appropriate as a database column we filter on? A soft delete of sorts that controls visibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was one approach i considered. Making schema changes, probably a new Repository method, changes to the query handler to call that method etc seemed like more to un-pick when the dependent permit records are finally deleted and this isnt needed anymore. Happy to re-factor in that pattern if you think its worthwhile though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say let's refactor to make it easier for us in the future. Decoupling the application from the data is also a positive.

As these agreements usually have a date associated with them, I'd recommend instead of a boolean of "hidden", let's throw a (nullable) date in there instead. Then in the code, if the row has a date populated do a comparison with today. This will enable us to hide these rows on a particular date (expiration of an agreement - which can be done on creation - then we'd not need this type of ticket at all).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem ill make the updates. 👍

}
29 changes: 26 additions & 3 deletions test/Olcs/src/Service/Data/IrfoGvPermitTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ public function provideFetchListOptions()

public function testFetchListData()
{
$results = ['results' => 'results'];
$results = ['results' => [['id' => 1],['id' => 2], ['id' => 12]]];
$expected = ['results' => [['id' => 1],['id' => 2]]];

$this->transferAnnotationBuilder->shouldReceive('createQuery')
->with(m::type(Qry::class))
Expand All @@ -72,8 +73,8 @@ public function testFetchListData()

$this->mockHandleQuery($mockResponse);

$this->assertEquals($results['results'], $this->sut->fetchListData());
$this->assertEquals($results['results'], $this->sut->fetchListData()); //ensure data is cached
$this->assertEquals($expected['results'], $this->sut->fetchListData()); //ensure data is cached
$this->assertEquals($expected['results'], $this->sut->fetchListData());
}

public function testFetchLicenceDataWithException()
Expand Down Expand Up @@ -121,4 +122,26 @@ protected function getSingleSource()
];
return $source;
}

public function testFilterArrayById()
{
$sourceData = [
['id' => 1, 'description' => 'Valid Entry'],
['id' => 5, 'description' => 'Should be filtered out'],
['id' => 10, 'description' => 'Valid Entry'],
['id' => 12, 'someField' => 'Should be filtered out'],
['id' => 19, 'description' => 'Should be filtered out'],
['id' => 22, 'otherField' => 'Valid Entry'],
];

$expected = [
['id' => 1, 'description' => 'Valid Entry'],
['id' => 10, 'description' => 'Valid Entry'],
['id' => 22, 'otherField' => 'Valid Entry']
];

$filteredData = $this->sut->filterArrayById($sourceData);

$this->assertEquals($expected, array_values($filteredData), "Filtered array does not match expected output");
}
}
Loading