Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update checkout tests with new factories #1645

Closed
wants to merge 4 commits into from

Conversation

itbabu
Copy link
Contributor

@itbabu itbabu commented Jan 24, 2015

I added fuctional tests to some checkout views: IndexView, ShippingAddressView, UserAddressUpdateView and DeleteUserAddressView.

There are some views left but in the meantime a test highlights that phonenumber is not working as expected with Python 3 and Django 1.6

    def test_submitting_valid_form_modifies_user_address(self):
        page = self.get(reverse('checkout:user-address-update', kwargs={'pk': self.user_address.pk}),
                        user=self.user)
        form = page.forms['update_user_address']
        form['first_name'] = 'Tom'
        response = form.submit()
        self.assertRedirectUrlName(response, 'checkout:shipping-address')
        self.assertEqual('Tom', UserAddress.objects.get().first_name)

This test will fail:

(Pdb) response
<200 OK text/html body=b'\n\n<!DO...l>\n'/14519>
(Pdb) response.form['phone_number'].value
'Country Code: 49 National Number: 3513296645 Country Code Source: 1 Preferred Domestic Carrier Code: '

It seems that the form is filled with the stringified phone number object and so the submit fails.

(Pdb) response.html
'(...)<span class="error-block"><i class="icon-exclamation-sign"></i> This is not a valid local or international phone format.</span>(...)'

I still have to figure out why this happens

- Update CheckoutMixin
- Add functional tests to IndexView, ShippingAddressView,
  UserAddressUpdateView and DeleteUserAddressView
@maiksprenger
Copy link
Member

Wow, lovely work. I'm having a look at the phone number issue now.

@maiksprenger
Copy link
Member

I lost wifi on the train and then got changed to a train without power outlets... I'll get to look at it on Friday.

@itbabu
Copy link
Contributor Author

itbabu commented Jan 28, 2015

I'll add few more tests in the next days.

Is there a reason to keep both assertRedirectUrlName and assertRedirectsTo assertions?

maiksprenger added a commit that referenced this pull request Jan 30, 2015
We have two methods doing something very similiar, so I'm removing the
one with the worse name.

Thanks go to @itbabu for spotting it in
#1645 (comment)
@maiksprenger
Copy link
Member

Marco, you're right, those two helpers are duplicates. I removed one here: 09f66c3

I also had a look at the unused delete code in ShippingAddressView and have to agree with you. As it's not really related to this PR, I took the liberty of cherry-picking it onto master here: 549decb I only modified the commit message.

Thanks for this. I'll investigate the phone number thing after lunch.

@maiksprenger
Copy link
Member

@itbabu, why are you suggesting adding unique IDs to the <form>s? I don't think there's harm in it; I'd just like to understand the reasoning.

maiksprenger added a commit that referenced this pull request Jan 30, 2015
Previously, we just used force_text() to serialise it. This somehow made
Django 1.6 print out the full representation (see #1645). It is the
wrong approach anyway, as we allow changing the standard display to a
regional format. But in the session we should always include the country
code. Luckily the easy fix is to just store things as an international
phone number.
maiksprenger pushed a commit that referenced this pull request Jan 30, 2015
* Uses factory-boy factories
* Adds helpers to the CheckoutMixin helper
* Adds plenty of functional tests

This commit is the squashed version of #1645 by @maikhoepfel, but it
does not contain the template changes. The unused
ShippingAddressView code was already cherry-picked across in 549decb.
maiksprenger pushed a commit that referenced this pull request Jan 30, 2015
* Uses factory-boy factories
* Adds helpers to the CheckoutMixin helper
* Adds plenty of functional tests
* Adds HTML IDs to some forms to be able to easily reference them
  from our tests

This commit is the squashed version of #1645 by @maikhoepfel, but it
does not contain the template changes. The unused
ShippingAddressView code was already cherry-picked across in 549decb.
@itbabu
Copy link
Contributor Author

itbabu commented Jan 30, 2015

@maikhoepfel It simplifes and make more understandable a functional test when we need to select a form inside a page.
Sometimes a page has lot of forms (see these tests: test_can_delete_a_user_address_from_shipping_address_page
test_can_select_an_existing_shipping_address)

There are already some tests in Oscar using this.
For example see

def test_can_delete_their_profile(self):
user_id = self.user.id
order_id = self.order.id
profile = self.app.get(reverse('customer:profile-view'),
user=self.user)
delete_confirm = profile.click(linkid="delete_profile")
form = delete_confirm.forms['delete_profile_form']
form['password'] = self.password
form.submit()

maiksprenger pushed a commit that referenced this pull request Jan 30, 2015
* Uses factory-boy factories
* Adds helpers to the CheckoutMixin helper
* Adds plenty of functional tests
* Adds HTML IDs to some forms to be able to easily reference them
  from our tests

@maikhoepfel:
This commit is the squashed version of #1645, but it
does not contain the template changes. The unused
ShippingAddressView code was already cherry-picked across in 549decb.

I also fixed some PEP8 issues.
maiksprenger added a commit that referenced this pull request Jan 30, 2015
PR #1645 pointed to an issue with phone numbers. After digging into the
code for a while, I opted to redo the phone number field implementation
according to the Django documentation. It always takes me a bit of time
to grok descriptors; the new implementation avoids them.
maiksprenger added a commit that referenced this pull request Jan 30, 2015
PR #1645 pointed to an issue with phone numbers. After digging into the
code for a while, I opted to redo the phone number field implementation
according to the Django documentation. It always takes me a bit of time
to grok descriptors; the new implementation avoids them.
maiksprenger added a commit that referenced this pull request Jan 30, 2015
PR #1645 pointed to an issue with phone numbers. After digging into the
code for a while, I opted to redo the phone number field implementation
according to the Django documentation. It always takes me a bit of time
to grok descriptors; the new implementation avoids them.

Note that this does not fix any issue; it's just a simplication done
while searching for the bug.
maiksprenger added a commit that referenced this pull request Jan 30, 2015
Django calls force_text on PhoneNumber instances when populating
a model form. Django 1.6's force_text implementation unfortunately
checks for __unicode__, which is defined in the base class.
As we're using the python_2_unicode_compatible decorator, in Python 2
__unicode__ is already overwritten by the decorator.
In Python 3, we can safely return __str__ because it returns unicode.

This was discovered by @itbabu in #1645.
maiksprenger added a commit that referenced this pull request Jan 30, 2015
Django calls force_text on PhoneNumber instances when populating
a model form. Django 1.6's force_text implementation unfortunately
checks for __unicode__, which is defined in the base class.
As we're using the python_2_unicode_compatible decorator, in Python 2
__unicode__ is already overwritten by the decorator.
In Python 3, we can safely return __str__ because it returns unicode.

This was discovered by @itbabu in #1645.
@maiksprenger
Copy link
Member

@itbabu, thanks for all the work and taking the time to explain.

I just pushed a squashed version of your work in a0ce8f2. Apart from some PEP8 changes and using assertRedirectsTo instead of the removed method, it's unchanged. I also finally managed to track down the bug you found: 9537531

I did quite a bit of cleanup along the way: 7b31081 and ee43070.

With that, I'm closing this PR. You're most welcome to add more tests; please create a new PR for that.

@itbabu itbabu deleted the checkout-tests branch February 1, 2015 18:56
itbabu added a commit to itbabu/django-oscar that referenced this pull request Feb 9, 2015
Adds new functional tests for ShippingAddressView, PaymentDetailsView
and ThankYouView.
It's the second part of django-oscar#1645 (be9d0d3)

Notes

* Adds a custom template to be able to test some functionalities of
  PaymentDetailsView
* Adds an helper to the CheckoutMixin
itbabu added a commit to itbabu/django-oscar that referenced this pull request Feb 9, 2015
Adds new functional tests for ShippingAddressView, PaymentDetailsView
and ThankYouView.
It's the second part of django-oscar#1645.

Notes

* Adds a custom template to be able to test some functionalities of
  PaymentDetailsView
* Adds an helper to the CheckoutMixin
mvantellingen pushed a commit that referenced this pull request Feb 22, 2015
Adds new functional tests for ShippingAddressView, PaymentDetailsView
and ThankYouView.
It's the second part of #1645.

Notes

* Adds a custom template to be able to test some functionalities of
  PaymentDetailsView
* Adds an helper to the CheckoutMixin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants