pablobm left a comment (openstreetmap/openstreetmap-website#6606)
I think this is now in a presentable state, even if objections could still
appear. Notes:
- The setting is still a `UserPreference`. Again, happy to move this to a new
column `users.public_heatmap`, but wanted to make sure first.
- In particular, I seem to recall there were some concerns about things
like accepting ToUs that can be only done on the website, so perhaps things
like that should progressively be moved to locations accessible by the API?
Just spitballing here. (/cc @1ec5)
- It occurred to me that the routes to edit the profile are under `show`
actions, when the convention would be to have them under `edit` actions. I've
stuck to `show`, but perhaps this should be changed in a different PR?
- I have not added system tests. I noticed these exist for the other profile
settings (eg:
[`test/system/profile_company_change_test.rb`](https://github.com/openstreetmap/openstreetmap-website/blob/master/test/system/profile_company_change_test.rb),
but I think they are redundant (we already cover them with controller tests)
and slow down the test suite. I would remove all of them (in a separate PR).
Thoughts?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6606#issuecomment-3660187507
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/6606/[email protected]>
_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
