• ZaphodBeeblebrox joined the channel
      • ZaphodBeeblebrox has quit
      • ZaphodBeeblebrox joined the channel
      • akashgp09 joined the channel
      • CatQuest has quit
      • akashgp09
        monkey: I print out the seriesDiffs but i can't see the diff for attributeSet in this array https://usercontent.irccloud-cdn.com/file/xcyCW...
      • monkey
        Right, that's issue #1 with the diffs, then.
      • To be honest I don't see why it's not working
      • Out of curiosity, you could see what the printout looks like if you make this method simply `return true`, that could possibly give us a clue https://github.com/bookbrainz/bookbrainz-data-j...
      • Bit of ashot in the dark, but…
      • And you could also have a look at the value of the jsonified baseData and otherData here: https://github.com/bookbrainz/bookbrainz-data-j...
      • akashgp09
        okay let me look.
      • > you could see what the printout looks like if you make this method simply `return true`
      • it outputs the same result as before.
      • This is how the jsonified output of otherdata and basedata looks like https://usercontent.irccloud-cdn.com/file/ks7Rh...
      • monkey
        I'm just seeing now that this is a Work revision. Is it possible that there is currently no way to add rel attributes for these relationships, and so the attributesSet value woudl be null, and not shown in the diff?
      • akashgp09
        maybe i can test it again by adding this line to workRevision diff `relationshipSet.relationships.attributeSet.relationshipAttributes.value`
      • monkey
        Ah, if it's not there that would 100% explain it
      • I thought you were using a series revision :p
      • So, yeah, that's definitely what it is
      • Otherwise these related models are not loaded
      • It ends up in `withRelated: includes` in the diffRevisions method
      • `const baseDataPromise = base.related('data').fetch({require: false, withRelated: includes});`
      • So this line `relationshipSet.relationships.attributeSet.relationshipAttributes.value` should probably be added to each entity revision type
      • akashgp09
        > I thought you were using a series revision :p
      • I am using series revision🤔
      • monkey
        Looking at the jsonified data you posted, it looks like the diff for a revision of this entity: https://bookbrainz.org/work/08bb8a6b-202b-4a9c-...
      • Unless I'm missing something
      • akashgp09
        A Pail of Air is a relationship within that series entity.
      • monkey
        Right, but the info I see is that of a workRevision, not a seriesRevision
      • To be clear, the revision in question will affect both the series and work entities, so for the same rev number you'll have a workRevision and a seriesRevision
      • akashgp09
      • Although now i haved added this code `relationshipSet.relationships.attributeSet.relationshipAttributes.value` to each models
      • but it still yields the same result
      • no attributeSet in diff.
      • monkey
        I'll try locally
      • akashgp09
        okay :)
      • monkey
        So I finally got to it, and I do see the attributes changes in the diff. Let me take a screenshot
      • So the top table is just the diff change paths, equivalent to the middle column of the second table (path) since there wasn't enough space to display it.
      • You'll see that attributeSetId has changed, along with the relationshipAttributes and their value
      • In the second table, the last two columns (lhs and rhs / left-hand side and right-hand side) show the value before and after the revision
      • Here's what I added to bookbrainz-site's src/server/routes/revison.js file:
      • So everything seems to be working okay as far as I can tell. The changes do need to be formatted and all that, but at least the diff has the appropriate changes I'm expecting to see.
      • So I think maybe you were only looking at the workRevision diffs and not the seriesRevision diffs
      • akashgp09
        oooooh yess 🤦
      • It is indeed showing the diffs for attributes. My bad i never tested it by editing the relationship.
      • I was only checking for newly created entity. https://usercontent.irccloud-cdn.com/file/3DHoD...
      • Thank you so much monkey.
      • monkey
        Right. the diffs only show changes, and if values on both sides are null (for example relationships with no attributes = null attributeSet) it won't show anything
      • No worries, at least we know this part of the diff works as expected, and while in there we've discovered that the entityRevisions all need to have that `relationshipSet.relationships.attributeSet.relationshipAttributes.value`, that's good progress
      • akashgp09
        > if values on both sides are null (for example relationships with no attributes = null attributeSet) it won't show anything
      • I guess even we add a new relationship( which have attributes value )[Kind ="n")] the diff doesn't include the path attributeSet.
      • monkey
      • If you then add an attribute, there will be an attributeSet with an id , that id will be ≠ null and will show in the diffs
      • And same if you then change the attribute value: new attributeSetID and attribute value will show in the diff
      • akashgp09
        But for newly added relationship(with attributes)[Kind="n"] will also have attributeSetId isn't it ?
      • monkey
        I'm not sure currently the attributeSetId will show up in the diffs
      • Here's the diffs I see for a revision where I added a new relationship to a series: https://usercontent.irccloud-cdn.com/file/ilbnC...
      • akashgp09
        column 3 right ?
      • monkey
        Column 3?
      • Not sure I understand
      • akashgp09
        sorry i mean, index 3 of second table
      • it shows new relationship is added
      • monkey
      • And so the relationshipSet will have a relationship which will have an attributeSetId
      • Here's what the relationships array looks like: https://usercontent.irccloud-cdn.com/file/AbIdx...
      • akashgp09
      • monkey
        Note the attributeSetId, confirming this relationship does have attributes
      • (relationshipAttributes is collapsed but it should have the attributes and their values)
      • So there will be some modifications needed in how we display new relationships too, in order to also show the attributes
      • akashgp09
        Yeah Got it.
      • monkey
        That would be in the `formatRelationshipAdd` method of the diffFormatters I'm guessing
      • Or `formatAddOrDeleteRelationshipSet`
      • Or both. I'm not so sure at this point :p
      • akashgp09
        Okay thanks. I will look into it. Will ping you if i have any doubts :p
      • monkey
      • akashgp09 has quit