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

Sync UK company and project site address fields on applicable projects #5929

Conversation

santoshdasa12345
Copy link
Contributor

@santoshdasa12345 santoshdasa12345 commented Jan 28, 2025

Description of change

We are replacing the old site decided question with a new one: is the [investment project] site address the same as the [uk recipient] company address? If users select "yes", the investment project's site address should be set to the uk company address; functionality that will be handled by the frontend before sending the PATCH request.

However, if the UK company address is subsequently updated, we wanted to ensure that applicable investment projects had their site address fields updated too; hence this PR.

Checklist

  • Has this branch been rebased on top of the current main branch?

    Explanation

    The branch should not be stale or have conflicts at the time reviews are requested.

  • Is the CircleCI build passing?

General points

Other things to check

  • Make sure fixtures/test_data.yaml is maintained when updating models
  • Consider the admin site when making changes to models
  • Use select-/prefetch-related field lists in views and search apps, and update them when fields are added
  • Make sure the README is updated e.g. when adding new environment variables

See docs/CONTRIBUTING.md for more guidelines.

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.64%. Comparing base (2c448f4) to head (428d61b).
Report is 46 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5929   +/-   ##
=======================================
  Coverage   96.64%   96.64%           
=======================================
  Files        1069     1069           
  Lines       25245    25259   +14     
  Branches     1664     1666    +2     
=======================================
+ Hits        24398    24412   +14     
  Misses        691      691           
  Partials      156      156           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@santoshdasa12345 santoshdasa12345 force-pushed the feature/CLS2-1174-sync-changes-to-companies-address-investment-project branch 2 times, most recently from 8a3de82 to 947d745 Compare February 3, 2025 16:17
@santoshdasa12345 santoshdasa12345 marked this pull request as ready for review February 3, 2025 16:17
@santoshdasa12345 santoshdasa12345 requested a review from a team as a code owner February 3, 2025 16:17
@santoshdasa12345 santoshdasa12345 force-pushed the feature/CLS2-1174-sync-changes-to-companies-address-investment-project branch from dbe64ac to b5e31f9 Compare February 4, 2025 12:08
@elcct
Copy link
Contributor

elcct commented Feb 4, 2025

Maybe it's worth exploring whether this is a good approach to keep a copy of company address in the investment project. Maybe downstream service could look up the company address if site_address_is_company_address is true instead or this could be "baked in" the serialiser.

@oliverjwroberts
Copy link
Contributor

Maybe it's worth exploring whether this is a good approach to keep a copy of company address in the investment project. Maybe downstream service could look up the company address if site_address_is_company_address is true instead or this could be "baked in" the serialiser.

@elcct I agree, it's not best practice to copy and store duplicate data. However, may I provide a little more context behind the intention of this change and reasons why I think this is still a "good enough" approach?

We are replacing the old site decided question with a new one: is the [investment project] site address the same as the [uk recipient] company address? If users select "yes", the investment project's site address should be set to the uk company address; functionality that will be handled by the frontend before sending the PATCH request.

However, if the UK company address is subsequently updated, we wanted to ensure that applicable investment projects had their site address fields updated too; something we thought best handled by signals and hence this PR.

Due to the fact the project site address can be either the company address, or another manually entered address, I think we still need to populate these fields either way, and make sure they stay up to date when the company address changes. Ensuring these fields are populated also allows us to piggy-back off existing functionality such as stage validation and minimise changes to analytical reporting, with fewer lines of code modified.

However, I would be happy to discuss further if you would like? In the meantime, I'll work with @santoshdasa12345 to make amendments as per your comments. Thanks.

@santoshdasa12345 santoshdasa12345 force-pushed the feature/CLS2-1174-sync-changes-to-companies-address-investment-project branch from c40ebde to 428d61b Compare February 5, 2025 18:09
@oliverjwroberts oliverjwroberts changed the title Add site address to company address Sync UK company and project site address fields on applicable projects Feb 10, 2025
@santoshdasa12345 santoshdasa12345 merged commit 95c1713 into main Feb 10, 2025
7 checks passed
@santoshdasa12345 santoshdasa12345 deleted the feature/CLS2-1174-sync-changes-to-companies-address-investment-project branch February 10, 2025 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants