CommunityCellularManager
CommunityCellularManager copied to clipboard
facebookarchive
CCM Enhancement sprint1 -updated
Hi Steve,
We have updated the code as per your comments. This sprint can not be grouped in small PRs as we have merged the entire code. But from sprint2 onwards we will logically group the requirements for PRs and deliver it accordingly.
Thanks.
Given that this is structured as 144 commits it would be possible to go back and create a number of smaller patches, but I understand that doing so would be a non-trivial amount of work.
@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
PLEASE address the comments I made by adding new commits to this PR rather than creating a whole new PR. If you create a new PR it's impossible to track the comments across the new commit(s).
@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Thanks for addressing my comments. A couple of high-level comments from @kkroo:
- please use the 'guardian' Django extension to handle permission checking: https://django-guardian.readthedocs.io/en/stable/api/guardian.templatetags.guardian_tags.html
- remove any debug/test code that is currently commented out
Don't worry about addressing the minor points at this time, let's just focus on those two things.
Thanks for the updates, will take a look. As discussed on the weekly call, we can't merge them until the changes to use Guardian have been made.
One process point: any time you have a sequence of git commits it should be possible to rewind to any point in that sequence and create a new branch, and thus a new PR. So technically, it is possible to break this PR apart, but at this point it would require a fair amount of work.
@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Per my comment on #45, please consider closing this PR and making all subsequent changes there. We aren't going to merge these changes until the Guardian integration is complete.