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

Bug - Community: Public Ownership Type -> Database Error #1370

Open
mdsimpson42 opened this issue Apr 3, 2024 · 7 comments
Open

Bug - Community: Public Ownership Type -> Database Error #1370

mdsimpson42 opened this issue Apr 3, 2024 · 7 comments
Labels
bug Something isn't working database Database design/migration

Comments

@mdsimpson42
Copy link
Contributor

Noticed when fixing another issue. When you try and save "Is the building in public/community ownership?" under Community, it produces a Database Error.

@mdsimpson42 mdsimpson42 added the bug Something isn't working label Apr 3, 2024
@mdsimpson42
Copy link
Contributor Author

I had a quick look into it. It's not due to missing columns in the database. I believe it is because we specify valid responses to the public ownership type in the SQL.

  • Maybe we should remove this validation step? It's a dropdown menu, after all.
  • The alternative is to update the list of valid responses, but that will require updating in the future if the values in the dropdown change again...

@mdsimpson42
Copy link
Contributor Author

There may be a bigger problem here. Existing data could be lost because we've changed the options without updating the existing data in the database.

@matkoniecz
Copy link
Contributor

Maybe we should remove this validation step? It's a dropdown menu, after all.

Note that someone can bypass client-side checks by making direct API calls

@mdsimpson42 mdsimpson42 added the database Database design/migration label Apr 5, 2024
@mdsimpson42
Copy link
Contributor Author

Okay, then we need to update the fields in the SQL as well, which will solve this bug.

It brings us back to the question of how to update existing data in the database. #1300

It does look like this has been addressed in the past by simply renaming the fields. When I was looking into this issue, I found this example in https://github.com/colouring-cities/colouring-core/blob/master/migrations/025.state_to_government.up.sql, where the field is simply renamed (without any attempt to preserve the edit history)

If this is all we need to do, then I can do that, which would unblock a whole load of other issues in Britain and Core.

But I don't really have enough expertise to do it in a way that will preserve the history. If it can be done for one of the issues linked above, I can then replicate the code and use it to solve the other issues, but I'm not confident enough to mess about with the live database when I don't really know what I'm doing.

@mdsimpson42
Copy link
Contributor Author

The old list of options, as verified in the SQL is:

  • 'Government-owned',
  • 'Charity-owned',
  • 'Community-owned/cooperative',
  • 'Owned by other non-profit body',
  • 'Not in public/community ownership'

The new options are:

  • "Public/State body",
  • "Public body with Private company",
  • "Charity",
  • "Community group/Cooperative",
  • "Other non-profit body",
  • "Privately owned company",
  • "Privately owned offshore company",
  • "Private individual",
  • "Other"

@polly64 I need to match up the old list to the new list.
Some are obvious (I think):

  • 'Government-owned' -> "Public/State body"
  • 'Charity-owned' -> "Charity"
  • 'Community-owned/cooperative' -> "Community group/Cooperative"
  • 'Owned by other non-profit body' -> "Other non-profit body"

But what about 'Not in public/community ownership'? Should I set it to "Privately owned company" for now? It doesn't look like there are that many entries in the database on production right now. Someone can always go through and manually correct them if necessary?

@mdsimpson42
Copy link
Contributor Author

Also, we're currently only visualising "Is the building in some form of public/community ownership" (Yes or no). Do we also want to visualise all of those options? It wouldn't take long to add that to the key.

@polly64
Copy link
Contributor

polly64 commented Apr 8, 2024

@mdsimpson42 yes great thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working database Database design/migration
Projects
None yet
Development

No branches or pull requests

3 participants