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

Fix select type replace aliases #251

Merged
merged 13 commits into from
Aug 20, 2021
Merged

Conversation

joshuaberetta
Copy link
Member

@joshuaberetta joshuaberetta commented Jun 22, 2021

Description

Fix bug that prevented select one being converted to select_one and handle legacy select one for exports and auto report to prevent failures.

Related issues

closes #250

@coveralls
Copy link

coveralls commented Jun 22, 2021

Coverage Status

Coverage decreased (-0.6%) to 85.486% when pulling df2ad69 on 250-fix-select-type-replace-aliases into a2bbc81 on master.

@joshuaberetta joshuaberetta force-pushed the 250-fix-select-type-replace-aliases branch from 2e3f853 to bc5e3d6 Compare June 22, 2021 00:32
@joshuaberetta joshuaberetta marked this pull request as ready for review June 22, 2021 17:08
@joshuaberetta joshuaberetta requested a review from dorey June 22, 2021 17:08
tests/test_exports.py Outdated Show resolved Hide resolved
@joshuaberetta joshuaberetta requested a review from dorey July 5, 2021 17:32
@dorey
Copy link
Contributor

dorey commented Jul 22, 2021

Out of curiosity, I checked out master a2bbc81 and ran the tests with the select_one_legacy fixture and test and it passed in most scenarios.

The one scenario that didn't pass was when v1.json had

        "type": "select one",
        "select_from_list_name": "emperors",

Have we already fixed this in master? Or is the bug triggered by this particular scenario?

@dorey
Copy link
Contributor

dorey commented Jul 22, 2021

Also I made a smaller test to see if the internal representation of the FormPack object was correct:

    def test_select_one_legacy_choice_list_linked(self):
        title, schemas, submissions = build_fixture('select_one_legacy')
        fp = FormPack(schemas, title)
        section = fp[0].sections['Your favourite Roman emperors']
        choice_list = section.fields['fav_emperor'].choice.options
        opt1_label = choice_list['julius']['labels'][None]
        # if this passes, then the choice list is associated
        assert opt1_label == 'Julius Caesar'

@joshuaberetta
Copy link
Member Author

@dorey sorry for letting this slip under the radar. Regarding this situation:

        "type": "select one",
        "select_from_list_name": "emperors",

This is the new buggy behaviour that I think my commit b98dbb1 introduced and this is on production. My fix here (as things stand) reverts back to the earlier behaviour of transforming this:

type name label
select one emperors emperor Who is your favourite emperor?

... into:

        "type": "select_one emperors",

When the now incorrect asset content contains:

        "type": "select one",
        "select_from_list_name": "emperors",

... the form-builder breaks that question and the export fails.

asset content
form-builder
export

This fix at least allows for the export to succeed even if the form-builder complains about the still incorrect asset content:

exports

If they upload their form again, the form-builder will function correctly as the new asset content is valid:

correct content
fixed form-builder

... and exports with all versions still works:

exports

If they now changed their XLSForm to the preferred select_one:

type name label
select_one emperors emperor Who is your favourite emperor?

... the asset content looks good with:

        "type": "select_one",
        "select_from_list_name": "emperors",

content

... the form-builder still works as expected:

form-builder

... and exports with all versions succeeds:

exports

as we see them (in expand_content)

* `expand_content` util has the
  dictionary available, and now replaces
  the select types if they are matched

* added one test of an alias of
  select_multiple
@dorey dorey merged commit f964aa9 into master Aug 20, 2021
@dorey dorey deleted the 250-fix-select-type-replace-aliases branch August 20, 2021 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some select-type questions not being replaced by their alias
3 participants