PR#258: Tests for Fasid Search using Mocking - fedora-commops/fedora-happiness-packets

fedora-commops / fedora-happiness-packets

#258 Tests for Fasid Search using Mocking

Merged 4 years ago by jflory7. Opened 4 years ago by alishapapun.

fedora-commops/ alishapapun/fedora-happiness-packets add/pytest-migrations into master

Test FASid search functionality using Mocking

Alisha Mohanty • 4 years ago

d95a06c

Write clean and maintanable code for fasidCheck

Alisha Mohanty • 4 years ago

81cf85c

assets/js/fas_details.js

file modified

+74 -59

		`@@ -1,72 +1,87 @@`
		`- $(document).ready(function () {`
		`+ $(function() {`
		`/*`
		`Place the button inside input field`
		`*/`
		`- $("#id_fasid").after($("#search_fasid"))`
		`+ $("#id_fasid").after($("#search_fasid"));`

		`/*`
		`Making the input fields clear when the page reloads`
		`*/`
		`- $("#id_fasid").val('')`
		`- $("#id_recipient_name").val('')`
		`- $("#id_recipient_email").val('')`
		`+ $("#id_fasid").val("");`
		`+ $("#id_recipient_name").val("");`
		`+ $("#id_recipient_email").val("");`

		`- $("#search_fasid").click(function () {`
		`+ $("#search_fasid").click(function() {`
		`+ let fasid = $("#id_fasid").val();`
		`+ $("#server-error").remove();`

		`- let fasid = $('#id_fasid').val()`
		`- $("#server-error").remove()`
		`-`
		`- if (fasid != '') {`
		`-`
		`- $("#id_recipient_name").attr("placeholder", "")`
		`- $("#id_recipient_name").val('')`
		`- $("#id_recipient_email").val('')`
		`- $("#id_fasid").prop('disabled', true);`
		`- $("#no-fas-id-error").remove()`
		`- $("#server-error").remove()`
		`- $("#div_id_fasid").after('<p class="searching-text">Searching for FAS Username.........</p>')`
		`-`
		`- email = $.get('/send/search', { fasid: fasid }, function (data) {`
		`-`
		`- if (data['server_error'] == 'True') {`
		`- $("#id_fasid").val('')`
		`- $('#server-error').remove()`
		`- $('.searching-text').remove()`
		`- $("#id_fasid").prop('disabled', false);`
		`- $("#div_id_fasid").after('<p class="error" id="server-error" >Internal Server Error Occured! Enter Name and Email manually!</p>')`
		`- $('#server-error').fadeIn('slow', function () {`
		`- $('#server-error').delay(3000).fadeOut()`
		`- })`
		`+ if (!fasid) {`
		`+ console.log("!fasid");`
		`+ return;`
		`+ }`
		`+ $("#id_recipient_name").attr("placeholder", "");`
		`+ $("#id_recipient_name").val("");`
		`+ $("#id_recipient_email").val("");`
		`+ $("#id_fasid").prop("disabled", true);`
		`+ $("#no-fas-id-error").remove();`
		`+ $("#server-error").remove();`
		`+ if (!$('#fas_username_check_message').length) {`
		`+ $("#div_id_fasid").after(`
		`+ '<p class="searching-text" id="fas_username_check_message">Searching for FAS Username...</p>'`
		`+ );`
		`+ }`
		`+`
		`+ email = $.ajax({`
		`+ type: "GET",`
		`+ url: "/send/search?fasid=" + encodeURIComponent(fasid),`
jflory7 commented 4 years ago This is a more elegant way of handling the FAS search. Nice work on cleaning this up. :thumbsup:
		`+ success: function(data) {`
		`+ console.log(data);`
		`+ $(".error").remove();`
		`+ $(".searching-text").remove();`
		`+ $("#id_fasid").prop("disabled", false);`
		`+ if (data["privacy"]) {`
		`+ $("#id_recipient_name").attr(`
		`+ "placeholder",`
		`+ "Privacy is Set! Type Name Manually"`
		`+ );`
		`+ } else {`
		`+ $("#id_recipient_name").val(data["name"]);`
		`}`
		`- else {`
		`- $('.error').remove()`
		`- $('.searching-text').remove()`
		`- $("#id_fasid").prop('disabled', false);`
		`-`
		`- if (data['account_exists'] == 'Yes') {`
		`-`
		`- if (data['name'] == 'No name') {`
		`- $("#id_recipient_name").attr("placeholder", "Privacy is Set! Type Name Manually")`
		`- }`

		`- else {`
		`- $("#id_recipient_name").val(data['name'])`
		`- }`
		`-`
		`- $("#id_recipient_email").val(data['email'])`
		`- }`
		`- else {`
		`- $("#no-fas-id-error").remove()`
		`- $("#div_id_fasid").after('<p class="error" id="no-fas-id-error">Sorry! No such FAS Username exist</p>')`
		`- console.log('No such FAS username exsists')`
		`- $('#no-fas-id-error').fadeIn('slow', function () {`
		`- $('#no-fas-id-error').delay(2700).fadeOut()`
		`- })`
		`- $("#id_fasid").val('')`
		`- }`
		`- return data;`
		`+ $("#id_recipient_email").val(data["email"]);`
		`+ },`
		`+ statusCode: {`
		`+ 404: function(data) {`
		`+ console.log(data);`
		`+ $("#no-fas-id-error").remove();`
		`+ $("#id_fasid").prop("disabled", false);`
		`+ $(".searching-text").remove();`
		`+ $("#div_id_fasid").after(`
		`+ '<p class="error" id="no-fas-id-error">Sorry! No such FAS Username exist</p>'`
		`+ );`
		`+ $("#no-fas-id-error").fadeIn("slow", function() {`
		`+ $("#no-fas-id-error")`
		`+ .delay(2700)`
		`+ .fadeOut();`
		`+ });`
		`+ $("#id_fasid").val("");`
		`+ },`
		`+ 500: function(data) {`
		`+ console.log(data);`
		`+ $("#id_fasid").val("");`
		`+ $("#server-error").remove();`
		`+ $(".searching-text").remove();`
		`+ $("#id_fasid").prop("disabled", false);`
		`+ $("#div_id_fasid").after(`
		`+ '<p class="error" id="server-error" >Internal Server Error Occured! Enter Name and Email manually!</p>'`
		`+ );`
		`+ $("#server-error").fadeIn("slow", function() {`
		`+ $("#server-error")`
		`+ .delay(3000)`
		`+ .fadeOut();`
		`+ });`
		`}`
		`- })`
		`- }`
		`- })`
		`+ }`
		`+ });`
		`+ });`
		`});`

happinesspackets/messaging/tests/test_mock_fasid.py

file added

+69

		`@@ -0,0 +1,69 @@`
		`+ import json`
		`+ from django.test.client import RequestFactory`
		`+ from ..views import FasidSearchView, AccountSystem`
		`+ import pytest`
		`+`
		`+`
		`+ @pytest.fixture(scope="function")`
		`+ def call_fasid_check_and_get_response(mocker, mock_response):`
		`+ factory = RequestFactory()`
		`+ request = factory.get('/send/search/', {'fasid': 'mockuser'})`
		`+ request.session = {} # hack for session middleware`
		`+ mocker.patch.object(AccountSystem, 'person_by_username', return_value=mock_response)`
		`+ return FasidSearchView.fasidCheck(request)`
		`+`
		`+`
		`+ @pytest.fixture(scope="function")`
		`+ def call_fasid_check_and_get_server_error(mocker):`
		`+ factory = RequestFactory()`
		`+ request = factory.get('/send/search/', {'fasid': 'mockuser'})`
		`+ request.session = {}`
		`+ mocker.patch.object(AccountSystem, 'person_by_username', side_effect=KeyError('human_name'))`
jflory7 commented 4 years ago Nice use of `side_effect` for this :thumbsup:
		`+ return FasidSearchView.fasidCheck(request)`
		`+`
		`+`
		`+ @pytest.mark.parametrize("mock_response", [{'privacy': False, 'email': 'mockuser@example.com', 'human_name': 'Mock User'}])`
		`+ def test_fasid_check_returns_email_name_and_privacy(call_fasid_check_and_get_response):`
		`+`
		`+ response = call_fasid_check_and_get_response`
		`+ expected_response = {`
		`+ 'privacy': False,`
		`+ 'email': 'mockuser@example.com',`
		`+ 'name': 'Mock User'`
		`+ }`
		`+`
		`+ assert response.status_code == 200`
		`+ assert json.loads(response.content) == expected_response`
		`+`
		`+`
		`+ @pytest.mark.parametrize("mock_response", [{'privacy': True, 'email': 'mockuser@example.com', 'human_name': 'None'}])`
		`+ def test_fasid_check_returns_null_in_name_when_user_has_set_privacy_to_true(call_fasid_check_and_get_response):`
		`+`
		`+ response = call_fasid_check_and_get_response`
		`+ expected_response = {`
		`+ 'privacy': True,`
		`+ 'email': 'mockuser@example.com',`
		`+ 'name': 'None'`
		`+ }`
		`+`
		`+ assert response.status_code == 200`
		`+ assert json.loads(response.content) == expected_response`
		`+`
		`+`
		`+ @pytest.mark.parametrize("mock_response", [None])`
		`+ def test_fasid_check_returns_404_when_fas_id_does_not_exist(call_fasid_check_and_get_response):`
		`+`
		`+ response = call_fasid_check_and_get_response`
		`+ expected_response = {'error': 'The FAS username does not exist!'}`
		`+`
		`+ assert response.status_code == 404`
		`+ assert json.loads(response.content) == expected_response`
		`+`
		`+`
		`+ def test_fasid_check_returns_500_for_an_unhandled_exception(call_fasid_check_and_get_server_error):`
		`+`
		`+ response = call_fasid_check_and_get_server_error`
		`+ expected_response = {'error': 'Internal Server Error'}`
		`+`
		`+ assert response.status_code == 500`
		`+ assert json.loads(response.content) == expected_response`
jflory7 commented 4 years ago Nice work. These tests are simple and easy to read. They are also pretty fast – another advantage to reusing code with fixtures. :thumbsup:

happinesspackets/messaging/views.py

file modified

+17 -21

		`@@ -252,32 +252,28 @@`
		`return queryset.filter(sender_email=self.request.user.email)`

		`class FasidSearchView():`
		`- @staticmethod`
		`def fasidCheck(request):`
		`try:`
		`fas = AccountSystem(username=settings.ADMIN_USERNAME, password=settings.ADMIN_PASSWORD)`
		`fasid = request.GET['fasid']`
		`- is_server_error = 'False'`
		`- type_of_error = ' No Error '`
		`person = fas.person_by_username(fasid)`
		`- u_name = 'No name'`
		`- u_email = 'No email'`
		`if not person:`
		`logger.error("The FAS username does not exist!")`
		`- account_exists = 'No'`
		`- else:`
		`- account_exists = 'Yes'`
		`- privacy = person['privacy']`
		`- if not(privacy):`
		`- logger.warn("The privacy is set to not view the Name!")`
		`- u_name = person['human_name']`
		`- u_email = person['email']`
		`- request.session['fasid'] = fasid`
		`- request.session['recipient_email'] = u_email`
		`- context = {'account_exists':account_exists,'email': u_email, 'name': u_name, 'server_error': is_server_error, 'type_of_error': type_of_error}`
		`+ response = JsonResponse({'error':'The FAS username does not exist!'})`
		`+ response.status_code = 404`
		`+ return response`
		`+ if person['privacy']:`
		`+ logger.warning("The privacy is set to not view the name!")`
		`+ user = {`
		`+ 'privacy': person['privacy'],`
		`+ 'email': person['email'],`
		`+ 'name': person['human_name']`
		`+ }`
		`+ request.session['fasid'] = fasid`
		`+ request.session['recipient_email'] = person['email']`
		`+ return JsonResponse(user)`
		`+`
		`except Exception as ex:`
		`- type_of_error = ex.__class__.__name__`
		`- logger.error("%s Occured", type_of_error)`
		`- is_server_error = 'True'`
		`- context = {'account_exists':'Can\'t Say','email': 'Can\'t Say', 'name': 'Can\'t Say', 'server_error': is_server_error, 'type_of_error': type_of_error}`
		`- return JsonResponse(context)`
		`+ response = JsonResponse({'error': 'Internal Server Error'})`
		`+ response.status_code = 500`
		`+ return response`
		`\ No newline at end of file`

requirements/base.txt

file modified

		`@@ -59,3 +59,4 @@`
		`pytest-django==3.5.0`
		`pytest==4.6.3`
		`pytest-cov==2.7.1`
		`+ pytest-mock==1.10.4`
		`\ No newline at end of file`

alishapapun commented 4 years ago

This PR address the following change
1. Refactors the Fasid Search Code, rewriting it into clean and maintainable code.
2. Add Tests for Fasid Search, using pytest-mock.

Hey, @jflory7 could you check the PR?

Metadata Update from @jflory7:
- Pull-request tagged with: PASSED, needs testing, new change, type - quality assurance, type - summer coding
- Request assigned

4 years ago

jflory7 commented 4 years ago

Hey @alishapapun, I should be able to review no later than Monday evening U.S. CDT. Thanks for sending this up! :thumbsup:

alishapapun commented 4 years ago

Hey @alishapapun, I should be able to review no later than Monday evening U.S. CDT. Thanks for sending this up! 👍

Sure @jflory7 .

jflory7 commented on line 67 of assets/js/fas_details.js 4 years ago

This is a more elegant way of handling the FAS search. Nice work on cleaning this up. :thumbsup:

jflory7 commented on line 32 of happinesspackets/messaging/views.py 4 years ago

Minor typo: net => not

jflory7 commented on line 40 of happinesspackets/messaging/views.py 4 years ago

:thumbsup:

jflory7 commented on line 31 of happinesspackets/messaging/tests/test_mock_fasid.py 4 years ago

Is expected_response dict needed or could you compare to mock_response dict? I don't think the contents will change here. Dropping expected_response will keep the test shorter.

jflory7 commented on line 50 of happinesspackets/messaging/tests/test_mock_fasid.py 4 years ago

Same comment here. I think you can compare this to mock_response and drop expected_response since they are identical.

jflory7 commented on line 7 of happinesspackets/messaging/tests/test_mock_fasid.py 4 years ago

This is a great opportunity to use a fixture for this method. Notice how you call this method in multiple tests? Fixtures are a handy way to repeat methods like this and give more control over how often they are called. Then, you pass the method name as a parameter to other tests that need it. I suggest making this fixture with this annotation:

@pytest.fixture(scope=module)

See more info about fixtures in this blog post.

Then, in the tests that need the request, you can change them to use the fixture in this way:

def test_fasid_check_returns_email_name_and_privacy(mocker, call_fasid_check_and_get_response):
    response = call_fasid_check_and_get_response

Does this make sense? I think using fixtures here will make these tests more robust.

Metadata Update from @jflory7:
- Pull-request untagged with: new change
- Pull-request tagged with: needs changes
- Request assigned

4 years ago

Metadata Update from @jflory7:
- Pull-request untagged with: needs testing
- Pull-request tagged with: new change

4 years ago

jflory7 commented 4 years ago

Nice work @alishapapun! :raised_hands: You kept these tests simple and straightforward. I left some feedback above with some suggestions. Let me know if my feedback makes sense.

1 new commit added

Fix error & Add pytest-mock as dependency

4 years ago

2 new commits added

Test FASid search functionality using Mocking
Write clean and maintanable code for fasidCheck

4 years ago

rebased onto 81cf85c

4 years ago

alishapapun commented 4 years ago

Hey @jflory7, I just made the following changes in the code since our code aims to be robust, in addition to the pytest fixture I added made some other changes like adding pytest.mark.parametrize to facilitate taking parameters in a better way. Lemme know if any more changes are required. Thank you