MozillaFoundation / donate-wagtail

[Legacy] - Wagtail based donation stack
Mozilla Public License 2.0
43 stars 22 forks source link

Some countries without Post-Code still have Post-Code set as required. #1518

Closed danielfmiranda closed 3 years ago

danielfmiranda commented 3 years ago

Describe the bug Based on feedback from a user, they have attempted to make a donation with Hong Kong set as the country, which removes the post code field from view and should make the field not required so the user can submit their donation.

However, if you fill out the form and attempt to make a donation, you will recieve an error message, and when changing to country with post code, will see that it is still expecting a post code.

To Reproduce Steps to reproduce the behavior:

  1. Go to the donation homepage
  2. Fill out the form
  3. Submit the form
  4. See error

Expected behavior It should allow the user to submit their donation without a post code

danielfmiranda commented 3 years ago

Interestingly, testing this on my local machine works as expected without a bug.

However, I am able to recreate the bug on staging.

I believe that the bug is in the back-end of the application, as all the front end is doing in regards to post code is removing the input from view. The back end is the portion that does the validation and will return a message if post-code is necessary.

I wonder if something when deployed is preventing the list from being loaded, thus making post code required for all countries?

danielfmiranda commented 3 years ago

Hi Everyone, here are some findings after troubleshooting this issue:

@Pomax I was wondering if you knew if there was something that is heroku specific that can be preventing the python file from reaching the post-codes-list information? Thanks in advance!

Pomax commented 3 years ago

If I repeat the STR on staging, I see the dev console throw an error about https://www.google-analytics.com/j/connect, and looking at the CSP_CONNECT_SRC, there is https://www.google-analytics.com/connect, so let's set that to include that /j/ and see what that does.

Pomax commented 3 years ago

That fixed the CSP warning, but not the original error. The heroku log also shows nothing wrong, so let's see if we can replicate this locally by using the exact same config that the stanging instance on heroku uses, with some tactical changes:

Aldo, the settings/staging.py has its Secure superclass removed, with:

Pomax commented 3 years ago

This works perfectly fine locally, so let's see if the DOM/source can tell us anything.

Pomax commented 3 years ago

So the error is indeed on the zip code field, which we still include, but hide:

image

Pomax commented 3 years ago

Looking at the error report:

image

Pomax commented 3 years ago

So, it would appear that the form on staging/production still thinks that for Hong Kong, zip code is required. Let's find out where that information is encoded.

Pomax commented 3 years ago

source/js/components/post-codes-list.json encodes this as:

{
  // ...
  {
     "abbrev":"HN",
     "name":"Honduras",
     "postal":"[0-9]{5}"
  },
  {
     "abbrev":"HK",
     "name":"Hong Kong"
  },
  {
     "abbrev":"HU",
     "name":"Hungary",
     "postal":"[0-9]{4}"
  },
  // ...
}
Pomax commented 3 years ago

We load this data in donate/payments/forms.py as:

# # Loading a JSON list of countries and their respective post code formats if applicable,
# # from the source/js directory.
try:
    with open('./source/js/components/post-codes-list.json') as post_code_data:
        COUNTRY_POST_CODES = json.load(post_code_data)
except Exception:
    COUNTRY_POST_CODES = None

After which we have the PostalCodeMixin mixin:

    def clean(self):
        cleaned_data = super().clean()
        postal_code = cleaned_data.get('post_code', '')
        country = cleaned_data.get('country', '')
        # If we cannot import post-code data, default to it being required.
        if COUNTRY_POST_CODES is None:
            self.check_post_code(postal_code)
        # Checking if the country uses post-code by finding it in JSON data.
        elif 'postal' in next(country_obj for country_obj in COUNTRY_POST_CODES if country_obj["abbrev"] == country):
            self.check_post_code(postal_code)

    def check_post_code(self, postal_code):
        if postal_code == "":
            raise forms.ValidationError({
                'post_code': _('This field is required.')
            })
Pomax commented 3 years ago
$ python
>>> import json
>>> with open('./source/js/components/post-codes-list.json') as post_code_data:
...     COUNTRY_POST_CODES = json.load(post_code_data)
... 
>>> country = "HK"       
>>> for country_obj in COUNTRY_POST_CODES:
...     if country_obj["abbrev"] == country:
...         print(country_obj)
... 
{'abbrev': 'HK', 'name': 'Hong Kong'}
>>> for country_obj in COUNTRY_POST_CODES:   
...     if country_obj["abbrev"] == country:
...         print('postal' in country_obj)
... 
False
>>> country = "IS"
>>> for country_obj in COUNTRY_POST_CODES:   
...     if country_obj["abbrev"] == country:
...         print('postal' in country_obj)
... 
True
Pomax commented 3 years ago
>>> if 'postal' in next(country_obj for country_obj in COUNTRY_POST_CODES if country_obj["abbrev"] == country):
...     print(f'{country} has a postal code requirement')
... 
IS has a postal code requirement
>>> country = "HK"
>>> if 'postal' in next(country_obj for country_obj in COUNTRY_POST_CODES if country_obj["abbrev"] == country):
...     print(f'{country} has a postal code requirement')
... 

So it should be reasonably evident that this code does what it's supposed to. HK does not have a postal attribute, and so check_post_code should not be kicking in.

if the country code JSON got loaded

Pomax commented 3 years ago

There is currently nothing that tells us whether or not the postal code data get read properly or not, so let's add in an investigatory print statement that will tell us whether, at dyno startup, we were able to read in the postal code data at all.

Pomax commented 3 years ago

A brief test: Bahrain, country code BH, also does not have a postal code. If the postal code data did not load on the python side, we would expect to see that fail too.

Pomax commented 3 years ago

it indeed fails:

image

Pomax commented 3 years ago

One more, just to make doubly sure. Jamaica, code JM, does not have a postal code requirement either.

Pomax commented 3 years ago

image

Pomax commented 3 years ago

Trying python on heroku directly seems to work fine:

Running bash on ⬢ donate-wagtail-staging... up, run.8918 (Standard-1X)
Generating SSH config
Done!
~ $ python
Python 3.7.8 (default, Sep 29 2020, 17:54:30)
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import json
>>> try:
...     with open('./source/js/components/post-codes-list.json') as post_code_data:
...         COUNTRY_POST_CODES = json.load(post_code_data)
... except Exception:
...     print('could not load postal code data')
...
>>> 

Same for running through the Django shell:

~ $ python manage.py shell
Python 3.7.8 (default, Sep 29 2020, 17:54:30) 
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> import os
>>> os.getcwd()
'/app'
>>> import json
>>> try:
...     with open('./source/js/components/post-codes-list.json') as post_code_data:
...         COUNTRY_POST_CODES = json.load(post_code_data)
... except Exception:
...     print('could not load postal code data')
...
>>> 
Pomax commented 3 years ago

Let's gather some evidence: https://github.com/mozilla/donate-wagtail/pull/1535