Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Bug: Unable to move first child after a sibling #76

Closed
JeremyCaney opened this issue Mar 26, 2021 · 3 comments
Closed

Bug: Unable to move first child after a sibling #76

JeremyCaney opened this issue Mar 26, 2021 · 3 comments
Assignees
@JeremyCaney
Labels
Area: Repositories Relates to the `ITopicRepository` interface or one of its implementations. Priority: 1 Severity 2: Major Status 5: Complete Task is considered complete, and ready for deployment. Type: Bug Behavior that is inconsistent with documented or expected behavior.
Milestone
OnTopic 5.1.0

Comments

@JeremyCaney
Copy link
Member

If a topic is the only child of its parent topic, it is unable to be moved after a sibling in another topic. If there are more than one topic in the parent topic, or it is moved to be the first child of the target topic, then it can be moved without any problems.
@JeremyCaney JeremyCaney self-assigned this Mar 26, 2021
@JeremyCaney JeremyCaney changed the title Bug: Unable to move some topics via Editor Bug: Unable to move only child after a sibling Mar 31, 2021
@JeremyCaney
Copy link
Member Author

This does not appear to be specific to the OnTopic Editor, and instead appears to be an issue with the underlying SqlTopicRepository.Move() implementation. This scenario seems to have been missed as part of the unit tests—which isn't surprising, given how specific it is. Despite that, we'll likely want to improve coverage since this is, apparently, a variable. Moving this issue to the OnTopic repository.

@JeremyCaney JeremyCaney transferred this issue from OnTopicCMS/OnTopic-Editor-AspNetCore Mar 31, 2021
@JeremyCaney JeremyCaney added Area: Repositories Relates to the `ITopicRepository` interface or one of its implementations. Priority: 1 Severity 2: Major Status 2: Scheduled Planned for an upcoming release. Type: Bug Behavior that is inconsistent with documented or expected behavior. labels Mar 31, 2021
@JeremyCaney JeremyCaney added this to the OnTopic 5.1.0 milestone Mar 31, 2021
@JeremyCaney
Copy link
Member Author

There’s an escape clause at the start of TopicRepository.Move() that attempts to validate whether the source topic is already placed after the sibling topic. It doesn’t actually validate that the source and sibling have the same parent, though, but simply compares their IndexOf() position.

If the sibling isn’t in the collection, it’s index will be -1; if the source is the first topic—as it will be if it’s the only topic—then it will have an index of 0. As these are one apart, it mistakenly interprets this as them already being siblings.

Adding an explicit check to validate their parent’s are the same will mitigate this issue, as would confirming that the sibling’s location isn’t less than zero. The former is the simplest approach.

@JeremyCaney JeremyCaney changed the title Bug: Unable to move only child after a sibling Bug: Unable to move first child after a sibling Apr 2, 2021
JeremyCaney added a commit that referenced this issue Apr 3, 2021
@JeremyCaney
Updated Move() escape clause to first check parent equality
8d1e1e4 
The escape clause at the top of `Move()` prevents further processing of the request if the topic is being moved to the same position it is already at. There was a bug in this, however, where it failed to check if the `target` was the same as the `topic.Parent`.

That works fine so long as the `topic` and the `sibling` are, indeed, in the same collection. But if they're not, and the `topic` is in the first position, this will lead to a false positive. In effect, this prevents the first topic from being moved after a sibling in another collection.

This is easily fixed by limiting the escape clause to cases where the current parent is the same as the `target` parent. This resolves #76, though we'll want to validate this via some unit tests.
JeremyCaney added a commit that referenced this issue Apr 3, 2021
@JeremyCaney
Introduced test to validate Move() with only-child to new parent, s…
4a13d0e 
…ibling

This validates the fix introduced for #76 (8d1e1e4).
JeremyCaney added a commit that referenced this issue Apr 3, 2021
@JeremyCaney
Introduced test to validate escape clause in Move()
858774f 
This test ensures that if a topic is being moved to the exact same place that it is already located, then the operation is canceled, and the `TopicMoved` event is never raised. This validates the logic that was failing as part of #76, but fixed in 8d1e1e4.
JeremyCaney added a commit that referenced this issue Apr 3, 2021
@JeremyCaney
Merge branch 'bugfix/TopicRepository.Move-first-child' into develop
eb0615f 
Fixed a bug where moving the first (or only) child of a topic to after a sibling in another topic would be inadvertently trip an escape condition meant to bypass cases where the topic was being moved to the same location that it was already located. As part of this, introduced two unit tests—one to validate that the bug no longer appears, and another to ensure that the escape condition still works. This resolves Issue #76.
@JeremyCaney JeremyCaney added Status 5: Complete Task is considered complete, and ready for deployment. and removed Status 2: Scheduled Planned for an upcoming release. labels Apr 3, 2021
@JeremyCaney
Copy link
Member Author

This was fixed by simply checking that the topic.Parent is the same as the target when evaluating whether the topic is being moved to the same location that it is already located (8d1e1e4). This is validated by two unit tests: one that confirms that the first topic can be moved after a sibling topic in another target (4a13d0e), and a second one that validates that confirms that the escape condition still works as expected (858774f). These updates were merged to develop as part of eb0615f.

@JeremyCaney JeremyCaney mentioned this issue Apr 7, 2021
Merged
JeremyCaney added a commit that referenced this issue Apr 12, 2021
@JeremyCaney
Merge branch 'release/5.1.0' into master
a4e8598 
OnTopic 5.1.0 is a minor release which introduces support for mapping constructor parameters (#35) and defining what model to use when mapping associations via the `[MapAs()]` attribute (#41). Primarily, however, it is focused on bug fixes, and resolves a number of priority issues, such as an exception which occurs when deleting topics with associations (#47), topics with deleted references being treated as _unresolved_ (#46), and the inability to move a first child to another parent (#76). Finally, it also includes a number of improvements, such as checking type compatibility before mapping an association (#83), migrating the unit tests to xUnit.net (#66), and establishing integration tests for ASP.NET Core (#39).

For a full rollup of new features, improvements, and bug fixes, see Pull Request #85.
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees

@JeremyCaney JeremyCaney

Labels
Area: Repositories Relates to the `ITopicRepository` interface or one of its implementations. Priority: 1 Severity 2: Major Status 5: Complete Task is considered complete, and ready for deployment. Type: Bug Behavior that is inconsistent with documented or expected behavior.
Projects
None yet
Milestone
OnTopic 5.1.0
Development

No branches or pull requests
1 participant
@JeremyCaney