CCM Enhancement sprint1 -updated #33
Conversation
This reverts commit 3103133.
Simplify management of CCM repo on GitHub.
This reverts commit 06c22aa.
This reverts commit 3b362c1.
This reverts commit b08541b.
…nto aricent_dev
Fix: Cloud Admin was available to Network Admin and Cloud Admin Fix: Username replaced with Email
Fix for user role permissions Code added to ajax request to get permission depending on role
Deleted Unwanted file
|
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. |
9muir
left a comment
9muir
left a comment
There was a problem hiding this comment.
These are just the first round of comments, more to come. Please don't add static third party files.
cloud/endagaweb/models.py
Outdated
|
|
||
| # To avoid duplicate names while running setup_test_db | ||
| # network = Network.objects.create() | ||
| network = Network.objects.create(name='Network_%s' % instance.pk) |
There was a problem hiding this comment.
When passing a single value to string formatting operator it's better to pass it as a 1-tuple: (instance.pk, )
cloud/endagaweb/models.py
Outdated
| network = Network.objects.create() | ||
|
|
||
| # To avoid duplicate names while running setup_test_db | ||
| # network = Network.objects.create() |
There was a problem hiding this comment.
better to just remove the old line rather than comment it out
There was a problem hiding this comment.
We haven't removed any of the old code which is changed or updated in case it is still required by your team.
Now removed.
cloud/endagaweb/models.py
Outdated
| profile = UserProfile.objects.create(user=instance) | ||
| network = Network.objects.create() | ||
|
|
||
| # To avoid duplicate names while running setup_test_db |
There was a problem hiding this comment.
change the comment to 'Add explicit name to avoid duplicate names when running setup_test_db`, to make it clear what you are doing.
There was a problem hiding this comment.
Updated with the mentioned comment
cloud/endagaweb/models.py
Outdated
| # instance.auth_group, created_group = Group.objects.get_or_create(name='network_%s' | ||
| # % instance.pk) | ||
| instance.auth_group, created_group = Group.objects.get_or_create(name='%s_GROUP_%s' | ||
| % (instance.name, instance.pk)) |
There was a problem hiding this comment.
convention would be to put the % at the end of the previous line (and make sure indentation of this line is correct).
cloud/endagaweb/models.py
Outdated
| # instance.auth_user, created_user = User.objects.get_or_create(username='network_%s' | ||
| # % instance.pk) | ||
| instance.auth_user, created_user = User.objects.get_or_create(username='%s_USER_%s' | ||
| % (instance.name, instance.pk)) |
cloud/endagaweb/views/dashboard.py
Outdated
|
|
||
| # Set the context with various stats. | ||
| content_type = ContentType.objects.filter(app_label='endagaweb', | ||
| model__in= |
There was a problem hiding this comment.
don't break the line here, but you can break after the .
There was a problem hiding this comment.
Fixed. Again it was done due to pylint configuration but for future will keep proper formatting.
cloud/endagaweb/views/dashboard.py
Outdated
| # Sending email now to reset password | ||
| try: | ||
| self._send_reset_link(request) | ||
| mail_info = 'Password reset Mail has been sent to %s'% email |
cloud/endagaweb/views/dashboard.py
Outdated
| print ex | ||
| mail_info = '\n Please configure email to send password reset ' \ | ||
| 'link to user' | ||
| messages.warning(request, mail_info,extra_tags="alert alert-danger") |
There was a problem hiding this comment.
needs whitespace after comma
cloud/endagaweb/views/dashboard.py
Outdated
| return JsonResponse( | ||
| {'status': 'success', 'message': 'User added successfully'}) | ||
|
|
||
|
|
cloud/endagaweb/views/dashboard.py
Outdated
| current_status = 'Blocked' | ||
|
|
||
| if (( | ||
| user_profile.user.is_superuser and user_profile.user.is_staff) |
There was a problem hiding this comment.
put and and or at ends of preceding lines
|
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). |
|
@aricent-ccm updated the pull request - view changes - changes since last import |
|
@9muir has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@aricent-ccm updated the pull request - view changes - changes since last import |
|
Thanks for addressing my comments. A couple of high-level comments from @kkroo:
Don't worry about addressing the minor points at this time, let's just focus on those two things. |
|
@aricent-ccm updated the pull request - view changes - changes since last import |
|
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. |
|
@aricent-ccm updated the pull request - view changes - changes since last import |
|
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. |
5 participants
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.