Skip to content

Conversation

@MAG-ElliotMorris
Copy link
Contributor

@MAG-ElliotMorris MAG-ElliotMorris commented Oct 2, 2025

We hadn't done this. Our entity data model has a lot of duplication and only slightly different paths with super-generic naming, so it manifests bugs like this, alas. Correct by construction please.

@MAG-AdrianMeredith Thank you for providing & anonymizing the test data, that made it much simpler to get this fix up.

@MAG-ElliotMorris MAG-ElliotMorris self-assigned this Oct 2, 2025
@github-actions
Copy link

github-actions bot commented Oct 2, 2025

🚀 Pull Request Review Guidelines

Thank you for taking the time to review this Pull Request. The following is a summary of our Pull Request guidelines. The full guidelines can be found here.

💬 How to Provide Feedback

We use a comment ladder when leaving review comments to avoid any ambiguity.

Tag Is response required? Is a change required? May the PR author resolve?
[Fix] ✔️ ✔️
[Consider] ✔️ ✔️
[Question] ✔️
[Nit] ✔️
[Comment] ✔️

All comments should be prefixed with one of the above tags, for example:
[fix] Your editor is still set to use tabs instead of spaces, or this file is old and needs to be reformatted.

⚠️ Reviewers should be sure to resolve conversations if they feel their feedback has been addressed sufficiently.

🎯 PR Author Focus Areas

  • Link Commits to Conversations After making a change in response to a reviewer's comment, link the specific commit in the comment thread. This will allow the reviewer to view the change without having to hunt the codebase for it.
  • Breaking Changes Any breaking changes made to the public interface must be specifically called out in the PR. The preferred format is a bullet list enumerating the specific nature of the change/s, as well as the types and method signatures that are affected. If necessary, migration guidance should also be provided here.

Thanks again for taking the time to review this Pull Request.

@MAG-ElliotMorris MAG-ElliotMorris force-pushed the work/OB-4721-no-parents branch 2 times, most recently from e6c4396 to a007e9d Compare October 2, 2025 13:28
Copy link
Contributor

@MAG-SamBirley MAG-SamBirley left a comment

Choose a reason for hiding this comment

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

If the test pases with this data,. we know we've solved the problem. Thanks for jumping on this one 🙏

Agreed on the code smell but I don't know enough about the shared behaviors between online and offline to suggest a neater solve. Got ideas for the future?

@MAG-ElliotMorris
Copy link
Contributor Author

MAG-ElliotMorris commented Oct 2, 2025

Alas no, not in the short term.

Long term its all about the whole flattening the entity hierarchy, changing/reducing the way we load things. All the stuff we've been talking with CHS about. It's definitely accidental complexity, not essential complexity.

We hadn't done this. Our entity data model has a lot of duplication and only slightly different paths it manifests bugs like this, alas. Correct by construction please.
@MAG-ElliotMorris MAG-ElliotMorris force-pushed the work/OB-4721-no-parents branch from a007e9d to b4ea779 Compare October 2, 2025 14:09
@MAG-ElliotMorris MAG-ElliotMorris marked this pull request as ready for review October 2, 2025 15:06
@MAG-ElliotMorris MAG-ElliotMorris requested a review from a team as a code owner October 2, 2025 15:06
@MAG-ElliotMorris MAG-ElliotMorris merged commit c693b4c into magnopus-opensource:main Oct 2, 2025
9 checks passed
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.

3 participants