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

Add initial support for unrelated relationship #339

Merged
merged 14 commits into from
Oct 9, 2020

Conversation

MebinAbraham
Copy link
Contributor

@MebinAbraham MebinAbraham commented Oct 7, 2020

What is the context of this PR?

Added the initial support unrelated relationship block.

The scope of this PR is just to add the initial screen with not specific functionality however there are quite a few places that doesn't work/haven't been accounted for as it does not impact this piece of work. For example the following are not correct:

  • previous location
  • next location
  • Location object serialisation

A lot of concepts haven't really been thought about and may reveal issues when implementing more of the functionality. Also, it might make sense to extend this piece to support more of the feature so that this is a bit more complete and makes more sense.

How to review

  • Is the page accessible manually
  • Is the content of the page correct?

Checklist

  • New static content marked up for translation
  • Newly defined schema content included in eq-translations repo

app/views/handlers/block_factory.py Show resolved Hide resolved
self.current_location.list_item_id, self.current_location.list_name
)
):
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

There should also be a check that the list name matches the for_list of the parent. I think otherwise in a schema with more than one list, it would be possible to use the unrelated page with the wrong list?

self.add_person("John", "Doe")
self.post({"anyone-else": "No"})

def test_relationship_unrelated_is_accessible_when_list_name_and_list_item_valid(
Copy link
Contributor

Choose a reason for hiding this comment

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

The test names could be made shorter without losing any clarity by removing relationship_unrelated / unrelated_relationship (as all of the tests in this class are testing unrelated relationships).

Copy link
Contributor

@LJBabbage LJBabbage left a comment

Choose a reason for hiding this comment

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

Husband or wife, or unrelated, what a question? I get the extra page and it looks good structurally, my only query is why if I use my list_item_id, I am giving an option to say I am related to myself, should it not be removed? The only other point is the url change, that we check that its ok.

@MebinAbraham
Copy link
Contributor Author

MebinAbraham commented Oct 9, 2020

Husband or wife, or unrelated, what a question? I get the extra page and it looks good structurally, my only query is why if I use my list_item_id, I am giving an option to say I am related to myself, should it not be removed? The only other point is the url change, that we check that its ok.

@LJBabbage

This card doesn't cover filtering of the list_summary as specified by the Trello card. This will simply list all householders. There is a card to filter the list_summary to only show the relevant people.

As for the URL, it was done after a discussion with @ajmaddaford. I did also bring it with the tech session as it makes things easier to deal with. But ultimately it was changed because the location object has a list_name attribute which is used by classes under inheritance so it made sense that the attribute to actually be part of the URL also.

@LJBabbage
Copy link
Contributor

This card doesn't cover filtering of the list_summary as specified by the Trello card. This will simply list all householders. There is a card to filter the list_summary to only show the relevant people.

As for the URL, it was done after a discussion with @ajmaddaford. I did also bring it with the tech session as it makes things easier to deal with. But ultimately it was changed because the location object has a list_name attribute which is used by classes under inheritance so it made sense that the attribute to actually be part of the URL also.

mmm, the filter according to the card is "to filter those who have not had a relationship established" its not quite the same

@MebinAbraham
Copy link
Contributor Author

MebinAbraham commented Oct 9, 2020

This card doesn't cover filtering of the list_summary as specified by the Trello card. This will simply list all householders. There is a card to filter the list_summary to only show the relevant people.
As for the URL, it was done after a discussion with @ajmaddaford. I did also bring it with the tech session as it makes things easier to deal with. But ultimately it was changed because the location object has a list_name attribute which is used by classes under inheritance so it made sense that the attribute to actually be part of the URL also.

mmm, the filter according to the card is "to filter those who have not had a relationship established" its not quite the same

Correct but it requires modifying how list_summary works, which is covered by https://trello.com/c/QNwNR874/4171-u-filter-list-summary-on-related-to-anyone-screen-m

- First iteration to use/display complete (unfiltered) `"list_summary": { "for_list": "household"`
- Subsequent card to allow a `filter` of the list_summary (to filter those who have not had a relationship established)

Have added a small comment to the filter card to filter out the current list item.

Copy link
Contributor

@LJBabbage LJBabbage left a comment

Choose a reason for hiding this comment

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

Not a problem, the original card just didn't say that

scripts/run_validator.sh Outdated Show resolved Hide resolved
@MebinAbraham MebinAbraham merged commit 12a6cfd into master Oct 9, 2020
@MebinAbraham MebinAbraham deleted the add-initial-support-for-unrelated-relationship branch October 9, 2020 12:13
@pricem14pc pricem14pc added this to the v3.51.0 milestone Oct 16, 2020
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.

4 participants