In commit 3648d34, why did you remove paginate_by = 5 in views.py? I did not follow this change.

This is a handy decorator. :smile:

Oh, I wonder why this was a 200 status code originally. Seems like a bug in the original test? :sweat_smile:

Just curious, why does response.status_code need to be split out separately?

Is there a reason you used unittest over django.test here? I ask because I was not sure myself and noticed django.test is used by other tests.

This is minor, but receipient => recipient.

receipient => recipient

Hey @alishapapun, nice work! :raised_hands:

This is almost ready to merge. I have a few questions above, and I also hoped you could walk me through commit ffc41ce.

It was awesome to see all of the test cases passing again. :smile: This should close #128.

bash-4.4# pytest
========================================================================= test session starts ==========================================================================
platform linux -- Python 3.6.8, pytest-4.6.3, py-1.8.0, pluggy-0.12.0
Django settings: happinesspackets.settings.dev (from environment variable)
rootdir: /app, inifile: setup.cfg
plugins: django-3.5.0, celery-4.2.1, cov-2.7.1
collected 63 items                                                                                                                                                     

happinesspackets/messaging/tests/test_models.py .                                                                                                                [  1%]
happinesspacket_schema_package/happinesspacket_schema/tests/test_schema.py ...........                                                                           [ 19%]
happinesspackets/messaging/tests/test_forms.py ..                                                                                                                [ 22%]
happinesspackets/messaging/tests/test_urls.py ..............                                                                                                     [ 44%]
happinesspackets/messaging/tests/test_views.py ..................................                                                                                [ 98%]
happinesspackets/utils/tests/test_misc.py .                                                                                                                      [100%]

Please take a look at my comments above. Pytest is also fairly new to me, so I am still learning too. I would also appreciate if you could explain what commit ffc41ce is doing, since I was not able to follow everything happening there.

In commit 3648d34, why did you remove paginate_by = 5 in views.py? I did not follow this change.

Earlier, the test test_named_message_indexed was not passing because of the paginate_by = 5, so I thought to remove it, but it seems pagination is necessary. So, added pagination in the latest commit and also added a test case test_pagination_is_ten to check if the search page is paginated. And the ensured the tests pass. Do give it a check @jflory7 :)

Oh, I wonder why this was a 200 status code originally. Seems like a bug in the original test? 😅

The response generated has status_code 200 initially, so in order to pass the test case, it's checked against status_code 200. So, I explicitly changed the status_code to 404 in case the Message doesn't exist. .

This is minor, but receipient => recipient.

Rectified the typo. 😅 in new commits

Is there a reason you used unittest over django.test here? I ask because I was not sure myself and noticed django.test is used by other tests.

Not a specific reason Justin. Since its better to maintain uniformity in all test cases I switched to django.test from unittest in the latest commits.

Hey @alishapapun, nice work! 🙌

Thanks @jflory7

This is almost ready to merge. I have a few questions above, and I also hoped you could walk me through commit ffc41ce.

See, in the commit earlier, the recipient_email was assigned to None while testing causing the TestCase to fail. Even if the request contain the recipient_email which was passed on to the forms method, it undergoes validation (validate_email_is_rate_limited) thus deleting the recipient_email value. Since validation is called before clean method, no entires of recipient_email is present when the scope comes to clean method, thus recipient_email value is assigned to None. (This type of error is not encountered by test_post_ratelimited_sender
since there was no validation. So, I put an if condition here which checks if the recipient_email value is present or not. I renamed the method validate-email to validate_email_is_rate_limited since, the method checks if messages count exceeds MAX_MESSAGES. Am I able to make you understand? Do let me know where more clarification is needed.

It was awesome to see all of the test cases passing again. 😄 This should close #128.
bash-4.4# pytest
========================================================================= test session starts ==========================================================================
platform linux -- Python 3.6.8, pytest-4.6.3, py-1.8.0, pluggy-0.12.0
Django settings: happinesspackets.settings.dev (from environment variable)
rootdir: /app, inifile: setup.cfg
plugins: django-3.5.0, celery-4.2.1, cov-2.7.1
collected 63 items

happinesspackets/messaging/tests/test_models.py . [ 1%]
happinesspacket_schema_package/happinesspacket_schema/tests/test_schema.py ........... [ 19%]
happinesspackets/messaging/tests/test_forms.py .. [ 22%]
happinesspackets/messaging/tests/test_urls.py .............. [ 44%]
happinesspackets/messaging/tests/test_views.py .................................. [ 98%]
happinesspackets/utils/tests/test_misc.py . [100%]

Please take a look at my comments above. Pytest is also fairly new to me, so I am still learning too. I would also appreciate if you could explain what commit ffc41ce is doing, since I was not able to follow everything happening there.

@alishapapun:
See, in the commit earlier, the recipient_email was assigned to None while testing causing the TestCase to fail. Even if the request contain the recipient_email which was passed on to the forms method, it undergoes validation (validate_email_is_rate_limited) thus deleting the recipient_email value. Since validation is called before clean method, no entires of recipient_email is present when the scope comes to clean method, thus recipient_email value is assigned to None. (This type of error is not encountered by test_post_ratelimited_sender since there was no validation.

Nice work on figuring out the root cause. :thumbsup:

@alishapapun:
So, I put an if condition here which checks if the recipient_email value is present or not. I renamed the method validate-email to validate_email_is_rate_limited since, the method checks if messages count exceeds MAX_MESSAGES. Am I able to make you understand? Do let me know where more clarification is needed.

I follow now. While this change makes the test pass, it comes at a cost of adding complexity. It is harder to understand what the clean() method is doing in forms.py. Adding if recipient_email gets the test to pass but does not solve the underlying problem.

I refactored two methods, clean() and is_recipient_email_equals_sender_email(). I'll write them first and then explain what I did:

def clean(self):
    super(MessageSendForm, self).clean()

    sender_emails = []
    sender_emails.append(self.user.email)
    sender_emails.append(strip_email(self.user.email))
    sender_emails.append(self.user.username + '@fedoraproject.org')
    recipient_emails = []
    recipient_emails.append(self.cleaned_data.get('recipient_email'))
    recipient_emails.append(strip_email(recipient_emails[0])

    if check_recipient_is_sender(sender_emails, recipient_emails):
        raise forms.ValidationError(
            "You cannot send a Fedora Happiness Packet to yourself!")
    elif self.cleaned_data.get('sender_approved_public_named') and not self.cleaned_data.get('sender_approved_public'):
        self.add_error('sender_approved_public_named', "If you want us to publish the message including your names, "
                                                        "you must also check 'I agree to publish this message and"
                                                        "display it publicly in the Happiness Archive'")
    validate_email_is_rate_limited(self.user.email)

def check_recipient_is_sender(sender_emails: List[str], recipient_email: List[str]) -> bool:
    if not set(sender_emails).isdisjoint(recipient_emails)
        return True
    else:
        return False

I made the check_recipient_is_sender() compare two lists for any common elements. In the clean() method, we add all sender and recipient emails to their own lists. Then we pass these lists to check_recipient_is_sender(). This makes check_recipient_is_sender() easier to test and also makes the code easier to read.

Also, I used a cool feature of Python 3.5 called typing (docs here, cheatsheet here). Typing creates type hints for expected data types. Since we always expect two lists in the above example and the method always returns a boolean, we can use typing for writing for robust unit tests (e.g. some tools will make sure object types are always correct). See this article for a longer explanation for why.

Lastly, to check if the two lists have overlapping content, I used isdisjoint() from Python built-in types (docs here). This is a better-performing way of comparing contents of two sets in Python.

This is a cleaner way of writing these two methods. It should make it easier to test, if it does not work with the existing tests already. I did not run the test suite with these changes. We should get the test to pass with the way these are written above. ^^

Does my feedback make sense? It was late when I typed this out, so let me know if you have doubts or if something is confusing.

@jflory7:
In commit 3648d34, why did you remove paginate_by = 5 in views.py? I did not follow this change.

@alishapapun:
Earlier, the test test_named_message_indexed was not passing because of the paginate_by = 5, so I thought to remove it, but it seems pagination is necessary. So, added pagination in the latest commit and also added a test case test_pagination_is_ten to check if the search page is paginated. And the ensured the tests pass. Do give it a check @jflory7 :)

Thanks for writing a test for this one! :thumbsup: It makes sense to me. However, in the Happiness Archive, pagination happens every five packets. Do you know why this is? I have a screenshot below from when I tested locally:

Squashed few commits, so had to force push the commits, now it turns the PR is now flooded with commits. :(

@alishapapun Props again on figuring this one out, this was tricky. :grin:

To clarify, is this PR ready for final review or is it still a work-in-progress? If it's ready, I can run through it Thursday evening US CDT time zone.

@alishapapun Props again on figuring this one out, this was tricky. 😁
To clarify, is this PR ready for final review or is it still a work-in-progress? If it's ready, I can run through it Thursday evening US CDT time zone.

It would be great if you test it once more since I just changed the test case for test_forms.py. After, its done, its all ready to get merged. And for the FASID-Search, I guess opening a new PR will be a good idea. What do you think @jflory7 ?

I ran through the newest changes and all tests pass. The tests are sensible to me. I believe we are ready for merging in the Pytest test suite! :raised_hands: Excellent work on turning this PR around @alishapapun. You covered a lot of ground and learned a few different skills to pull this off. I hope you are proud of your work here too.

Before merging, could you please squash all commits down to one and write a summary of changes in the single squashed commit? Once the commits are squashed, we can merge it! :smile:

This commit address the following changes :100:
- Modify clean method in forms.py
- Add test case for Pagination in Search View
- Add unit test for check_recipient_is_sender (Test sender email equals to recipient email)
- Pass the TestClass TestMessageSenderConfirmationView
- Pass the TestCase function of test_post_ratelimited_sender
- Work on user authentication (previously not working) in Testing
- Migrate tests to pytest
- Rename Tests to match Pytest norms.
I guess all good for merging :ocean: @jflory7 :)

Super. Let's ride that wave right into master! :sunglasses: :joy:

Merging!