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

Remove Python Code legacy which was used for the old form #3380

Closed
3 tasks done
karlcow opened this issue Jul 13, 2020 · 2 comments · Fixed by #3384
Closed
3 tasks done

Remove Python Code legacy which was used for the old form #3380

karlcow opened this issue Jul 13, 2020 · 2 comments · Fixed by #3384
Assignees

Comments

@karlcow
Copy link
Member

karlcow commented Jul 13, 2020

  • Find code which is only used for the old form
  • Remove it
  • Double check if there are relevant tests
@karlcow karlcow self-assigned this Jul 13, 2020
@karlcow
Copy link
Member Author

karlcow commented Jul 13, 2020

Rewriting the python code will be funkier than I initially thought, because the wizard form code is not entirely independent of the initial form. And there has been duplication in some parts of the code.

For example, this defines the original and the wizard form.

problem_choices = [
('detection_bug', 'Desktop site instead of mobile site'),
('site_bug', 'Site is not usable'),
('layout_bug', 'Design is broken'),
('video_bug', 'Video or audio doesn\'t play'),
('unknown_bug', 'Something else')
]
problem_choices_wizard = [
('detection_bug', 'svg-problem-mobile-vs-desktop.svg',
'Desktop site instead of mobile site'),
('site_bug', 'svg-problem-no-use.svg', 'Site is not usable'),
('layout_bug', 'svg-problem-broken-design.svg', 'Design is broken'),
('video_bug', 'svg-problem-no-video.svg', 'Video or audio doesn\'t play'),
('unknown_bug', 'svg-problem-question.svg', 'Something else')
]

BUT here the wizard form is using the category of the original form

problem_category = PrefixedRadioField(
[InputRequired(message=radio_message)],
choices=problem_choices_wizard
)

And this line is also using the old datastructures

return get_radio_button_label(category, problem_choices).lower()

With a function which expect the 2 values instead of 3 values.

def get_radio_button_label(field_value, label_list):
"""Return human-readable label for problem choices form value."""
for value, text in label_list:
if value == field_value:
return text
# Something probably went wrong. Return something safe.
return 'Unknown'

Fun ahead.

@karlcow
Copy link
Member Author

karlcow commented Jul 13, 2020

We currently have form code in

  • webcompat/helpers.py, webcompat/views.py, and webcompat/form.py

The most important lines on the route issues/new with regard to the form is:

form_data = prepare_form(request)
bug_form = get_form(form_data, form=FormWizard)

  • prepare_form(request) prepares a form_data dictionary with the all the information that could be extracted from the HTTP request. It's not that much a prepare_form but more a extract_request_data
    def prepare_form(form_request):
    """Extract all known information from the form request.
    This is called by /issues/new to prepare needed by the form
    before being posted on GitHub.
    For HTTP POST:
    The JSON content will override any existing URL parameters.
    The URL parameters will be kept if non-existent in the JSON.
    """
    form_data = {}
    form_data['user_agent'] = request.headers.get('User-Agent')
    form_data['src'] = request.args.get('src')
    form_data['extra_labels'] = request.args.getlist('label')
    form_data['url'] = request.args.get('url')
    # we rely here on the fact we receive the right POST
    # because we tested it with form_type(request)
    if form_request.method == 'POST':
    json_data = form_request.get_json()
    form_data.update(json_data)
    return form_data
  • bug_form = get_form(form_data, form=FormWizard) passes the dictionary previously extracted and choose the type of forms to modelize the data. This information is then used by the template rendering
    def get_form(form_data, form=IssueForm):
    """Return an instance of flask_wtf.FlaskForm.
    It receives a dictionary of everything which needs to be fed to the form.
    """
    bug_form = form()
    ua_header = form_data['user_agent']
    # Populate the form
    bug_form.browser.data = get_browser(ua_header)
    # Note: The details JSON that was POSTed to the new issue endpoint is at
    # this point a Python dict. We need to re-serialize to JSON when we store
    # its value in the hidden details form element, otherwise when we attempt
    # to decode it as JSON on form submission, it will throw (because Python
    # dicts are not valid JSON)
    bug_form.details.data = json.dumps(form_data.get('details'), indent=2)
    bug_form.extra_labels = form_data.get('extra_labels', None)
    bug_form.os.data = get_os(ua_header)
    bug_form.reported_with.data = extract_report_source(form_data)
    bug_form.ua_header.data = ua_header
    bug_form.url.data = form_data.get('url', None)
    return bug_form

karlcow added a commit to karlcow/webcompat.com that referenced this issue Jul 14, 2020
These are breadcrumbs of dead code and useless redirection of variables.

Let's remove them before modifying the Form.
karlcow added a commit to karlcow/webcompat.com that referenced this issue Jul 14, 2020
This optimizes a bit the code to make it easier to read. It doesn't change the global logic
karlcow added a commit to karlcow/webcompat.com that referenced this issue Jul 14, 2020
Originally the list of tuples were 2 items long.
but the wizard is using in some cases 3 items long list (because of icons) for radio fields.

This leds to the duplication of the list of problem_choices with 2 items and 3 items. So instead of duplicating code, let's handle it in the function itself. There might be something more elegant to do. We can discover that later.
karlcow added a commit to karlcow/webcompat.com that referenced this issue Jul 14, 2020
So this is doing a couple of things.

- It removes the duplicate problem_choices list of 2 items tuples.
- it removes the IssueForm class
- it transfers the IssueForm attributes into FormWizard
- it reorders in some broad categories the fields  for the form
- it switches getform to FormWizard. (it was not the case!)
- it adjusts the tests
@karlcow karlcow linked a pull request Jul 14, 2020 that will close this issue
karlcow added a commit to karlcow/webcompat.com that referenced this issue Jul 14, 2020
Originally the list of tuples were 2 items long.
but the wizard is using in some cases 3 items long list (because of icons) for radio fields.

This leds to the duplication of the list of problem_choices with 2 items and 3 items. So instead of duplicating code, let's handle it in the function itself. There might be something more elegant to do. We can discover that later.
karlcow added a commit to karlcow/webcompat.com that referenced this issue Jul 14, 2020
So this is doing a couple of things.

- It removes the duplicate problem_choices list of 2 items tuples.
- it removes the IssueForm class
- it transfers the IssueForm attributes into FormWizard
- it reorders in some broad categories the fields  for the form
- it switches getform to FormWizard. (it was not the case!)
- it adjusts the tests
miketaylr pushed a commit that referenced this issue Jul 15, 2020
Fixes #3380 - Makes Wizard Form the default (python)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant