[openstreetmap/openstreetmap-website] add icon for OSMF membership to user page (#1914)
notifications at github.com
Sat Feb 23 10:37:01 UTC 2019
I was asked to review this today. Apart from my inline comments, my thoughts are:
* I would suggest using lowercase throughout for 'OSMF', such as in enums, translations keys, factory names etc. If we add another membership in the future with more complex capilitization (FoOBaR) then it would be less to remember to just have everything in lowercase.
* I would prefer to see the tests split up as described into smaller blocks that are a bit more clear as to what is being tested each time. But I would accept the PR as-is since it's fairly similar to the other tests we have.
* Some of the functionality here is squeezed into the user account page, so we end up with a not-very-rails-like way of iterating over the memberships to save the 'show' status. I'd prefer to see this refactored, but that can come later, since the whole account page needs refactoring anyway.
* I'm fine with seeing these in a different table, since I can see that they have a different set of attributes from the user_roles table, and are only being used for badges rather than as part of the permissions system.
* I would consider using a mixture of controller and system tests, instead of an integration test, but I would accept this PR as-is.
I think the final call on the new table vs user_roles table should be made by TomH rather than me.
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the rails-dev