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

Use primary block for generating primary edit links #219

Merged
merged 7 commits into from
Jul 30, 2020

Conversation

iwootten
Copy link
Contributor

@iwootten iwootten commented Jul 28, 2020

What is the context of this PR?

The links generated on the summary page are incorrect for primary person, meaning that it will break if only a primary person is entered and then the change link is used on the summary to update their details. We've confirmed it is not necessary for the question to be different when editing the primary person.

How to review

Add only a primary person to household, proceed to summary, try to edit that person. See if you agree with the changes.

@iwootten
Copy link
Contributor Author

Copy link
Contributor

@MebinAbraham MebinAbraham left a comment

Choose a reason for hiding this comment

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

Bug fixed 👍 Just a few minor thoughts.

Comment on lines 221 to 225
return [
block
for group in section["groups"]
for block in group["blocks"]
if block["type"] == "ListCollector" and block["for_list"] == for_list
if block["type"] == collector_type and block["for_list"] == for_list
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be nice if this returned a generator since any time we currently use this, it only grabs the first block.

Comment on lines 98 to 105
primary_person_blocks = self._schema.get_list_collectors_for_list(
section, for_list=summary["for_list"], primary=True
)
primary_person_edit_block_id = (
primary_person_blocks[0]["add_or_edit_block"]["id"]
if len(primary_person_blocks) > 0
else None
)
Copy link
Contributor

Choose a reason for hiding this comment

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

We could save fetching the primary person list collector block by checking if the list store for the for_list contains a primary person. I think we should also use a generator.

To add to that, if we check the list count, then we can use the normal list collector when we know it is available, i.e. when count is above 1, therefore a better UX given as it changes the text for the user.

        primary_person_edit_block_id = None
        current_list = self._list_store[summary["for_list"]]
        if len(current_list) == 1 and current_list.primary_person:
            primary_person_edit_block_id = self._schema.get_list_collectors_for_list(
                section, for_list=summary["for_list"], primary=True
            )[0]["add_or_edit_block"]["id"]

@ajmaddaford
Copy link
Contributor

The change link for primary person on a list collector goes to the list collector edit screen. This may be acceptable, but we should check that it is. If so, we should implement Mebin's suggestion so that the change link from the section summary behaves in the same way.

@pricem14pc pricem14pc modified the milestones: v3.46.0, v3.47.0 Jul 30, 2020
@iwootten
Copy link
Contributor Author

This may be acceptable, but we should check that it is.

Checked with Laura and she's confirmed that this is ok.

@iwootten iwootten merged commit 1982be9 into master Jul 30, 2020
@iwootten iwootten deleted the wlh-breaking-on-change-primary branch July 30, 2020 14:24
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.

5 participants