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

Prevent duplicate answer_ids across different list collectors #228

Merged

Conversation

liamtoozer
Copy link
Contributor

@liamtoozer liamtoozer commented Dec 5, 2023

What is the context of this PR?

An answer ID is globally unique in a schema, except for list collectors with the same name when used in one or more locations - it is correct that validation allows for this.

However, when there are multiple list collectors with a different name in a schema, the answer ID needs to be unique for each list collector child block (add, edit, and remove). Validation needs to be in place to ensure this.

The consequence of not validating this is that when Runner loads a schema with 2 list collectors, but both share an answer_id in the add_block, edit_block, or remove_block, if there is a routing rule in place to skip the first list collector then both list collectors are removed from the path, instead of just the first.

Additionally, we can also validate to make sure that a list collector child block answer ID is not used anywhere else in the schema if it's not for the same list.

Changes for this PR:

  • I've updated the validate_other_list_collectors to include all list collectors in a schema so that we can also validate against those.
  • As mentioned in the PR comment by Katie, we can also validate to ensure that the answer IDs in child blocks for every list collector are unique for the answer_ids across the rest of the schema

How to review

To review, try to validate the schema linked in the Trello ticket - it should now show the new validation error message.

Would appreciate any advice/feedback on improvements too!

@liamtoozer
Copy link
Contributor Author

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Only functionally tested so far, but

  • if I change the invalid example added in this PR so that the visitor LC has visitor-first-name I get no errors 👍
  • If I then change the edit block to edit-visitor-first-name I get "Different list collectors populate a list using duplicate answer_ids in the add block" However the error is actually that the edit block and add block don't match, not that different LCs have duplicates, I think something has gone wrong?

Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Ignore above, it does correctly pick up add/edit not matching 👍
There is a case that has slipped through, add/edit don't allow duplicates in different LCs now but two different list collectors could have the same answer-id in the remove block, might need small extension for this case

@liamtoozer liamtoozer added the Bug Fix Something isn't working label Dec 8, 2023
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Just a few minor comments 👍
Also this doesn't prevent having
block-1 with answer-1
list-collector with add-block with answer-1

Might not need fixing in this card? but it is a case that could get through

app/validators/blocks/list_collector_validator.py Outdated Show resolved Hide resolved
app/validators/blocks/list_collector_validator.py Outdated Show resolved Hide resolved
app/validators/blocks/list_collector_validator.py Outdated Show resolved Hide resolved
Update key names for error.
Add new unit test for this scenario.
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Couple tiny non blockers, but all working for me now 👍

app/validators/blocks/list_collector_validator.py Outdated Show resolved Hide resolved
app/validators/blocks/list_collector_validator.py Outdated Show resolved Hide resolved
app/validators/blocks/list_collector_validator.py Outdated Show resolved Hide resolved
@MebinAbraham
Copy link
Contributor

MebinAbraham commented Dec 12, 2023

Just a few minor comments 👍 Also this doesn't prevent having block-1 with answer-1 list-collector with add-block with answer-1

Might not need fixing in this card? but it is a case that could get through

We should handle this given we are in this space. Happy for it to be a follow-up card to a separate PR. @liamtoozer can you raise a card or PR to fix this please but progress it next.

@liamtoozer
Copy link
Contributor Author

Just a few minor comments 👍 Also this doesn't prevent having block-1 with answer-1 list-collector with add-block with answer-1
Might not need fixing in this card? but it is a case that could get through

We should handle this given we are in this space. Happy for it to be a follow-up card to a separate PR. @liamtoozer can you raise a card or PR to fix this please but progress it next.

Just to confirm - this should hopefully be covered in this PR now. Updated the PR description to reflect this

Copy link
Contributor

@berroar berroar left a comment

Choose a reason for hiding this comment

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

Tested, LGTM 👍

@liamtoozer liamtoozer merged commit b012742 into main Dec 19, 2023
3 checks passed
@liamtoozer liamtoozer deleted the prevent-duplicate-answer-ids-across-different-list-colectors branch December 19, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants