Skip to content

Fix person content type folderish behaviour - #467

Merged
iFlameing merged 3 commits into
mainfrom
person-back
Aug 7, 2026
Merged

Fix person content type folderish behaviour#467
iFlameing merged 3 commits into
mainfrom
person-back

Conversation

@iFlameing

Copy link
Copy Markdown
Member

No description provided.

@iFlameing
iFlameing requested a review from a team as a code owner August 5, 2026 14:31

@reekitconcept reekitconcept left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In general this is good and it fixes the real issue correctly, but I'd like to ask you make two changes:

  1. Add a test! There are zero tests for the Person serializer, which is exactly why #354's regression shipped unnoticed. Without one, the next person to touch this file reintroduces it. Minimal version, but instead of adding it myself please add it and test it out:
def test_person_is_folderish(self, portal):
    with api.env.adopt_roles(["Manager"]):
        person = api.content.create(
            container=portal, type="Person",
            id="jane-doe", given_name="Jane", family_name="Doe",
        )
        api.content.create(container=person, type="Image", id="portrait")
    result = getMultiAdapter((person, portal.REQUEST), ISerializeToJson)()
    assert result["is_folderish"] is True
    assert result["items_total"] == 1
    # the vocabulary enrichment must survive
    assert "locations" in result
    assert "organisational_units" in result

The last two assertions are the important ones. They pin both invariants together, so a future change can't trade one for the other.

  1. Change the news entry name to 467.bugfix : when there is a ticket number we should use it so it shows in the changelog. We use the +personIsFolderish...bugfix format only when there is no ticket.

Please ask me to review the final again when you're done!

@iFlameing

Copy link
Copy Markdown
Member Author

@reekitconcept i added the test and rename the changelog file.

@iFlameing
iFlameing merged commit fad4688 into main Aug 7, 2026
24 checks passed
@iFlameing
iFlameing deleted the person-back branch August 7, 2026 07:33
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.

2 participants