You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Editing comments (in TermUI and WebUI) has no effect #653
Reverting this line back to commentId = op.target seems to fix the issue. But I'm not sure if this is the right way to solve the problem. This discussion seems to be related, maybe reverting the line was just forgotten?
2415515 was correct. The problem is that there is a conversion missing.
EditCommentOperation correctly use an Operation's Id as the target. The problem is that the termui (and webUI I suppose) is using a comment's ID (a combined ID) directly without conversion. Where exactly this conversion should happen, I'm not so sure.
Also, having a different type (entity.CombinedId) might make sense to avoid those class of bugs. We would know what kind of Id we have and the compiler could enforce the correct usage.
Tough problem. I added a CombinedId type and replaced the comment.id type (entity.Id) with CombinedId. I also adjusted the function signatures accordingly. But one Problem persists. The TimeLineItem interface requires an Id, but the CommentTimeLineItem uses a CombinedId, as it should match the Comment.Id (of type CombinedId).
...is using a comment's ID (a combined ID) directly without conversion.
Also, having a different type (entity.CombinedId) might make sense to avoid those class of bugs. We would know what kind of Id we have and the compiler could enforce the correct usage.
If that works without introducing other complications, then that would help a lot, I guess.
If it were up to me I would aim for separate types for different kinds of Ids. But in case that does not work out as expected, one can add a field plainId to the Comment struct to store both Ids for each Comment. Then one has always the correct Id at his disposal.
FYI I started working on that in #664 by having a dedicated CombinedId type and use that in various places, but I'm yet to figure out the clean way to join in the middle. Should be doable though.
@MichaelMure I fiddled around with your PR on my own fork. I think I found the Problem with the WebUI.
The WebUI passes the CommentId (the TimelineId of type CombinedId) to the Target of the EditCommentMutation.
This Id was then passed wrongly around. Instead the Resolver has to extract the OperationId of the CommentId and pass this OpId around. I fixed this and now the WebUI fails with the same error as the TermUI.
One problem left, the extracted OpId is only a Prefix and will fail the id validation due to "invalid length"... :'(
(Possible relevant note by Michael was given here: GlancingMind#57 (comment))
Got it! Editing of comments via CLI, TermUI and WebUI work again. GlancingMind#57GlancingMind#61
Unfortunately the fix looks rather bad... duplicating the same code in three four different places screams for a better solution.
I could remove the redundant code by placing it into EditCommentOperation::Apply, but then the EditCommentOperationTarget
must be a CombinedId. Making the Target a TimelineItem rather than an Operation.
Side Note: Is the separation of a TimelineItem and an Operation needed or could an Operation not be a TimelineItem?
#835 brought to light the bug that likely cause this. Somewhere in the code an EditCommentOperation was created, the Id() was taken to register the change in the timeline, then metadata were added to that operation, changing the data and the Id. A newly created assertion failed in that branch, fix pending.
Activity
GlancingMind commentedon Apr 25, 2021
Reverting this line back to
commentId = op.targetseems to fix the issue. But I'm not sure if this is the right way to solve the problem. This discussion seems to be related, maybe reverting the line was just forgotten?Fix git-bug#653
Fix git-bug#653
MichaelMure commentedon Apr 26, 2021
2415515 was correct. The problem is that there is a conversion missing.
EditCommentOperation correctly use an Operation's Id as the target. The problem is that the termui (and webUI I suppose) is using a comment's ID (a combined ID) directly without conversion. Where exactly this conversion should happen, I'm not so sure.
Also, having a different type (
entity.CombinedId) might make sense to avoid those class of bugs. We would know what kind of Id we have and the compiler could enforce the correct usage.GlancingMind commentedon Apr 26, 2021
Tough problem. I added a CombinedId type and replaced the comment.id type (entity.Id) with CombinedId. I also adjusted the function signatures accordingly. But one Problem persists. The TimeLineItem interface requires an Id, but the CommentTimeLineItem uses a CombinedId, as it should match the Comment.Id (of type CombinedId).
I don't know how they they should be converted. 😬
rng-dynamics commentedon Apr 26, 2021
If that works without introducing other complications, then that would help a lot, I guess.
rng-dynamics commentedon Apr 26, 2021
If it were up to me I would aim for separate types for different kinds of Ids. But in case that does not work out as expected, one can add a field
plainIdto theCommentstruct to store both Ids for eachComment. Then one has always the correct Id at his disposal.bug: have a type for combined ids, fix #653
MichaelMure commentedon May 3, 2021
FYI I started working on that in #664 by having a dedicated
CombinedIdtype and use that in various places, but I'm yet to figure out the clean way to join in the middle. Should be doable though.bug: have a type for combined ids, fix git-bug#653
GlancingMind commentedon Jun 2, 2021
@MichaelMure I fiddled around with your PR on my own fork. I think I found the Problem with the WebUI.
The WebUI passes the CommentId (the TimelineId of type CombinedId) to the Target of the EditCommentMutation.
This Id was then passed wrongly around. Instead the Resolver has to extract the OperationId of the CommentId and pass this OpId around. I fixed this and now the WebUI fails with the same error as the TermUI.
One problem left, the extracted OpId is only a Prefix and will fail the id validation due to "invalid length"... :'(
(Possible relevant note by Michael was given here: GlancingMind#57 (comment))
GlancingMind commentedon Jun 2, 2021
Got it! Editing of comments via CLI, TermUI and WebUI work again.
GlancingMind#57GlancingMind#61Unfortunately the fix looks rather bad... duplicating the same code in
threefour different places screams for a better solution.I could remove the redundant code by placing it into EditCommentOperation::Apply, but then the EditCommentOperationTarget
must be a CombinedId. Making the Target a TimelineItem rather than an Operation.
Side Note: Is the separation of a TimelineItem and an Operation needed or could an Operation not be a TimelineItem?
bug: have a type for combined ids, fix git-bug#653
bug: have a type for combined ids, fix git-bug#653
bug: have a type for combined ids, fix #653
bug: have a type for combined ids, fix #653
MichaelMure commentedon Jul 25, 2022
#835 brought to light the bug that likely cause this. Somewhere in the code an EditCommentOperation was created, the Id() was taken to register the change in the timeline, then metadata were added to that operation, changing the data and the Id. A newly created assertion failed in that branch, fix pending.
bug: have a type for combined ids, fix #653
Merge pull request #664 from MichaelMure/combined-id-rework