CCM Enhancement sprint1 -updated - Githubissues

facebookarchive / CommunityCellularManager

Tool for deploying, managing and controlling your Community Cellular Networks

Other

87 stars 36 forks source link

CCM Enhancement sprint1 -updated #33

Open piyushabad88 opened 7 years ago

piyushabad88 commented 7 years ago

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.

9muir commented 7 years ago

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.

facebook-github-bot commented 7 years ago

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

9muir commented 7 years ago

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).

facebook-github-bot commented 7 years ago

@aricent-ccm updated the pull request - view changes - changes since last import

facebook-github-bot commented 7 years ago

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented 7 years ago

@aricent-ccm updated the pull request - view changes - changes since last import

9muir commented 7 years ago

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.

facebook-github-bot commented 7 years ago

@aricent-ccm updated the pull request - view changes - changes since last import

9muir commented 7 years ago

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.

facebook-github-bot commented 7 years ago

@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot commented 7 years ago

@aricent-ccm updated the pull request - view changes - changes since last import

9muir commented 7 years ago

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.