Skip to content

Conversation

@rsk2
Copy link
Contributor

@rsk2 rsk2 commented Dec 9, 2023

For #311

@fallen fallen linked an issue Dec 10, 2023 that may be closed by this pull request
@fallen fallen self-requested a review December 10, 2023 11:41
Copy link
Member

@fallen fallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!
This looks good, just a few remarks in the review :)
Also, could you add/update the testsuite to test for those new fields?
Thanks a lot!

{% block extracss %}
<link href="{% static "css/petition.css" %}" rel="stylesheet" type="text/css">
{% if petition.has_share_buttons %}
{% if petition.has_email_share_button or petition.has_facebook_share_button or petition.has_tumblr_share_button or petition.has_linkedin_share_button or petition.has_twitter_share_button or petition.has_mastodon_share_button or petition.has_whatsapp_share_button %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could add a has_any_share_button() method in the Petition class that does this boolean computation, just for the sake of keeping the template easy to read.

</ul>
</div>
{% endif %}
{% if petition.has_email_share_button or petition.has_facebook_share_button or petition.has_tumblr_share_button or petition.has_linkedin_share_button or petition.has_twitter_share_button or petition.has_mastodon_share_button or petition.has_whatsapp_share_button %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark here about adding a has_any_share_button() method to Petition class.

{% block extrajs %}
<script type="text/javascript" src="{% static "js/petition.js" %}"></script>
{% if petition.has_share_buttons %}
{% if petition.has_email_share_button or petition.has_facebook_share_button or petition.has_tumblr_share_button or petition.has_linkedin_share_button or petition.has_twitter_share_button or petition.has_mastodon_share_button or petition.has_whatsapp_share_button %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same remark here.

@fallen
Copy link
Member

fallen commented Dec 17, 2023

For the unit test you can either add a new test in pytition/petition/tests/tests_EditPetitionView.py or just modify test_edit_petition_POST_social_network_form().
I would see it like this:

  • Testing that submitting the form with one of the has_xxx_share_button set and that the Petition object then has the correct property value
  • Then unset it and submit the form again and check that the property is unset
  • Do this for each new property.
  • You can kind of copy paste those tests in pytition/petition/tests/tests_PetitionTemplateViews.py and just modify the name of the view to submit the edit_template form instead of the edit_petition so that the new properties get tested for the Template model class as well.

@rsk2
Copy link
Contributor Author

rsk2 commented Dec 19, 2023

For the unit test you can either add a new test in pytition/petition/tests/tests_EditPetitionView.py or just modify test_edit_petition_POST_social_network_form(). I would see it like this:

OK. I am planning to add new test in the file pytition/petition/tests/tests_EditPetitionView.py

* Testing that submitting the form with one of the `has_xxx_share_button` set and that the Petition object then has the correct property value

@fallen can you please give me an example test for one of the has_xxx_share_button?

@fallen
Copy link
Member

fallen commented Dec 21, 2023

@fallen can you please give me an example test for one of the has_xxx_share_button?

Sure, here you go:

    def test_edit_petition_email_share_button(self):
        julia = self.login('julia')
        org = Organization.objects.get(name='RAP')
        share_button_form_data = {
            'social_network_form_submitted': 'yes',
            'has_email_share_button': True,
        }

        # For an org owned petition
        p = Petition.objects.create(title="My petition", org=org)

        # By default, a new petition has no share button
        self.assertFalse(p.has_email_share_button)

        # Now let's enable the email share button
        response = self.client.post(reverse("edit_petition", args=[p.id]),
                                    share_button_form_data)
        self.assertEqual(response.status_code, 200)
        p.refresh_from_db()
        self.assertTemplateUsed(response, "petition/edit_petition.html")
        self.assertEquals(response.context['social_network_form'].is_valid(), True)
        self.assertEquals(response.context['social_network_form'].is_bound, True)
        self.assertEquals(response.context['content_form_submitted'], False)
        self.assertEquals(response.context['email_form_submitted'], False)
        self.assertEquals(response.context['social_network_form_submitted'], True)
        self.assertEquals(response.context['newsletter_form_submitted'], False)
        self.assertTrue(p.has_email_share_button)

        # Let's now turn it back off
        share_button_form_data['has_email_share_button'] = False
        response = self.client.post(reverse("edit_petition", args=[p.id]),
                                    share_button_form_data)
        self.assertEqual(response.status_code, 200)
        p.refresh_from_db()
        self.assertFalse(p.has_email_share_button)

        # For a user owned petition
        p = Petition.objects.create(title="My petition 2", user=julia)

        # By default, a new petition has no share button
        self.assertFalse(p.has_email_share_button)

        # Now let's enable the email share button
        share_button_form_data['has_email_share_button'] = True
        response = self.client.post(reverse("edit_petition", args=[p.id]), share_button_form_data)
        self.assertEqual(response.status_code, 200)
        self.assertTemplateUsed(response, "petition/edit_petition.html")
        p.refresh_from_db()
        self.assertEquals(response.context['social_network_form'].is_valid(), True)
        self.assertEquals(response.context['social_network_form'].is_bound, True)
        self.assertEquals(response.context['content_form_submitted'], False)
        self.assertEquals(response.context['email_form_submitted'], False)
        self.assertEquals(response.context['social_network_form_submitted'], True)
        self.assertEquals(response.context['newsletter_form_submitted'], False)
        self.assertTrue(p.has_email_share_button)

        # Let's now turn it back off
        share_button_form_data['has_email_share_button'] = False
        response = self.client.post(reverse("edit_petition", args=[p.id]), share_button_form_data)
        self.assertEqual(response.status_code, 200)
        p.refresh_from_db()
        self.assertFalse(p.has_email_share_button)

This tests that when creating a Petition, the "has_email_share_button" is by default "off" and then tries to enable it and switch it back off using the "edit_petition" view.
This is done twice: once for an organization owned Petition and once for a user owned Petition.

I hope you are well and I wish you a very happy festive season :)

@rsk2
Copy link
Contributor Author

rsk2 commented Dec 24, 2023

@fallen Wish you a very happy festive season too!

@rsk2
Copy link
Contributor Author

rsk2 commented Feb 7, 2024

Hi @fallen, hope all is well. The PR is ready for review and please take a look whenever you get the chance.

Copy link
Member

@fallen fallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks good to me, thanks a lot and sorry for the review delay!

@fallen fallen merged commit ebeeea5 into pytition:master Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Allow to chose which social network sharing button to show or not

2 participants