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

Add View Licence contact details page #993

Merged
merged 23 commits into from
May 13, 2024

Conversation

jonathangoulding
Copy link
Collaborator

@jonathangoulding jonathangoulding commented May 8, 2024

https://eaflood.atlassian.net/browse/WATER-4433

The existing service handling view licence is slow because it loads all the data for the tabs in one render. Work has been done previously to refactor the summary page to load only the summary information.

This change will introduce a contact details controller, service and presenter to handle the view licence contact details page.

This will share the same view as the summary page and load the same 'common data' established in previous work.

https://eaflood.atlassian.net/browse/WATER-4433
https://eaflood.atlassian.net/browse/WATER-4434

The existing service handling view licence is slow because it loads all the data for the tabs in one render. Work has been done previously to refactor the summary page to load only the summary information.

This change will introduce a contact details controller, service and presenter to handle the view licence contact details page.

This will share the same view as the summary page and load the same 'common data' established in [previous work](#957).
@jonathangoulding jonathangoulding self-assigned this May 8, 2024
@jonathangoulding jonathangoulding marked this pull request as ready for review May 9, 2024 13:48
@jonathangoulding jonathangoulding requested review from Cruikshanks and robertparkinson and removed request for Cruikshanks May 9, 2024 14:02
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

Putting this in just to record I'm still looking at this PR rather than this is all I found!

I'm digging into the Fetch as behind the scenes I can see it's running some big queries but I need a sec to get a handle on what it is currently doing before I feel I can pass any judgement.

test/controllers/licences.controller.test.js Show resolved Hide resolved
jonathangoulding and others added 2 commits May 10, 2024 14:02
…details-page

# Conflicts:
#	app/controllers/licences.controller.js
#	test/controllers/licences.controller.test.js
Cruikshanks added a commit that referenced this pull request May 10, 2024
https://eaflood.atlassian.net/browse/WATER-4433

This relates to [Add View Licence contact details page](#993). What started as experimenting with an alternate fetch implementation has grown into a detailed comparison of the original changes and our current patterns and conventions.

In this we have tried to highlight how we think the changes would be implemented if they followed our current norms.

These are suggested and can be challenged. But we felt in this case "show don't tell" would be more appropriate to support the conversation!
Cruikshanks added a commit that referenced this pull request May 10, 2024
https://eaflood.atlassian.net/browse/WATER-4433

This relates to [Add View Licence contact details page](#993). What started as experimenting with an alternate fetch implementation has grown into a detailed comparison of the original changes and our current patterns and conventions.

In this we have tried to highlight how we think the changes would be implemented if they followed our current norms.

These are suggested and can be challenged. But we felt in this case "show don't tell" would be more appropriate to support the conversation!
Cruikshanks added a commit that referenced this pull request May 13, 2024
https://eaflood.atlassian.net/browse/WATER-4433

This relates to [Add View Licence contact details page](#993). What started as experimenting with an alternate fetch implementation has grown into a detailed comparison of the original changes and our current patterns and conventions.

In this we have tried to highlight how we think the changes would be implemented if they followed our current norms.

These are suggested and can be challenged. But we felt in this case "show don't tell" would be more appropriate to support the conversation!
Cruikshanks added a commit that referenced this pull request May 13, 2024
https://eaflood.atlassian.net/browse/WATER-4433

This relates to [Add View Licence contact details page](#993). What started as experimenting with an alternate fetch implementation has grown into a detailed comparison of the original changes and our current patterns and conventions.

In this we have tried to highlight how we think the changes would be implemented if they followed our current norms.

These are suggested and can be challenged. But we felt in this case "show don't tell" would be more appropriate to support the conversation!
Cruikshanks added a commit that referenced this pull request May 13, 2024
https://eaflood.atlassian.net/browse/WATER-4433

This relates to [Add View Licence contact details page](#993). What started as experimenting with an alternate fetch implementation has grown into a detailed comparison of the original changes and our current patterns and conventions.

In this we have tried to highlight how we think the changes would be implemented if they followed our current norms.

These are suggested and can be challenged. But we felt in this case "show don't tell" would be more appropriate to support the conversation!
@Cruikshanks Cruikshanks added enhancement New feature or request labels May 13, 2024
Cruikshanks and others added 2 commits May 13, 2024 12:39
In this commit, we have tried to highlight how we think the changes would be implemented if they followed our current norms.

- corrections - adding line breaks, extensions to module imports, parens and block body for arrow functions
- fetch logic - switching from Objection to Knex directly. Where we are fetching data from various tables to provide a summarised view, and not intending to interact with the instances other than that, dropping to Knex can make things easier and remove the need for additional logic in presenters services
- single responsibility - switching the basis for the modules to be single-responsibility. The ViewService will be responsible for fetching and presenting the data for the whole tab. But assume fetching and presenter licence contacts versus customer contacts will be different hence rename/update the fetch and presenter to be licence contact-specific
@jonathangoulding jonathangoulding merged commit 1e94502 into main May 13, 2024
5 of 6 checks passed
@jonathangoulding jonathangoulding deleted the view-licence-contact-details-page branch May 13, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants