BB-444: Revision Display Page Improvement (WIP)

[prabalsingh24]

WIP
PR not ready for review rn

[prabalsingh24]

I changed the BBID to AliasName. What other changes should be made on this page @MonkeyDo ?

I think the way relationship is shown here is not very good. I added four relationships to "God Emperor of Dune" and the bbids in the table are of this work itself. I think it would be better if those bbids were of the other entity whom which this work is related to.

Even though "Public Affairs" and "Wonder Woman" were revised, their changes are not shown ( a bug maybe).

What do you think?

[chinmay-kothari]

@prabalsingh24
Writing AliasName there may not look good when we have long AliasNames like: https://beta.bookbrainz.org/edition/4e4788aa-b139-47e3-8674-dad226ddc9aa

What do you think?

[MonkeyDo]

It definitely looks better. Maybe it would be good to also show the BBID? maybe under the title in a <small> tag?

I agree the way relationships are displayed is terrible. There's lots of low hanging fruits to make that page better.
To display relationships, in an ideal world I would love something like the diagram in the help page, but that might be for another day.

As for @chinmay-kothari 's concern about long names, you'll have to make sure the name wraps correctly, but I don't think we have many other options.

[MonkeyDo]

Even though "Public Affairs" and "Wonder Woman" were revised, their changes are not shown ( a bug maybe).

Possibly. How did you make changes to the relationships of God Emperor of Dune as well as Public Affairs and Wonder Woman at the same time?
In any case, it looks like it would be worth investigating.

[prabalsingh24]

I was editing "God Emperor of Dune" and I kept adding relationships.

[coveralls]

Coverage decreased (-0.3%) to 49.97% when pulling 9a0db14 on prabalsingh24:revision-display into 1c50ebf on bookbrainz:master.

[prabalsingh24]

formatNewRelationshipSet function will be used when you add relationships to an entity w/o any previous relationships.
Edit: https://beta.bookbrainz.org/revision/13170 In this revision, relationships are now shown because all those 5 entities didn't have a relationshipSet before.
edit: changed this to formatAddOrRemoveRelationpshipSet

[prabalsingh24]

The formatRemoveRelationships function isn't perfect yet...
It's having bug in following case.
Let's say the old relationship set is having following ids``[100 , 200, 300, 400, 500, 600] and the new relationship set is [100, 200, 400, 500, 600]
Deep-Diff library which is used to find the changes will show that elements at 2nd, 3rd, 4th index are edited. and the element in 5th index (600) is deleted but in reality element 300 was deleted..

The formatAddRelationships is working fine because every time a new relationship is created, it's relationshipId will always greater than the older ids (I think?) and because of it, it will always be at the last index ( because we are sorting before finding the diffs)

[prabalsingh24]

Instead of showing bbid of the entity itself, I changed it to show bbid of the other entity. the one it's related to. (check table)
I made a mistake in the column name, I am gonna fix it in thenext commit. Don't wanna create an extra commit for a typo :(

[prabalsingh24]

@MonkeyDo Tagging you because the option to ask for a review is not showing up :P

[MonkeyDo]

This is already a lot better than what was there before, well done!
It's courageous to attack this small mountain of cryptic code that is the diff formatters!

I think this will be a good first step, but ultimately we'll want to implement more changes.
For one, as you mentioned, changes in the order of arrays are not well handled by deep-diff.
We may be able to solve this by sorting elements by id before comparing, or we may want to move away to another library.
I find in particular that identifier diffs are very annoying that way
This comment suggests there might be a way to ignore the order of keys (maybe needs updating the package)
This issue has a few candidates for library replacement (for another day)

I also think there's a better way to display relationships, but it'll take some time to design and implement. @anirudhjain75 suggested in a GSOC proposal to visualize them as a graph, which I think has a lot of potential but will be pretty dense work and needs a clear path.
I'd love something that looks similar to the image that is on the BB help page

[prabalsingh24]

In this revision, I removed MusicBrainz ID, but this shows that I removed WikiData and then
added WikiData inplace of MusicBrainz ID.

Deep-Diff isn't working properly. Should it be replaced w some different library?

[MonkeyDo]

In this revision, I removed MusicBrainz ID, but this shows that I removed WikiData and then
added WikiData inplace of MusicBrainz ID.

Deep-Diff isn't working properly. Should it be replaced w some different library?

Same as before, then. That's unfortunate.
I wonder if there's a way to sort the items by id before doing the comparison, and whether that would help.

Would it help to print out the data of two specific revisions (before/after) to be able to test it in a sandbox?

[prabalsingh24]

We ARE sorting it by ids. https://github.com/bookbrainz/bookbrainz-data-js/blob/1b3bf5c12326a8419401f283e25e014aca917347/src/util.js#L158

how about we compare relationshipSet and IdentifierSet by brute force. Picking up one relationship from the old set and check if it's present in the new set? Code might get little messy but it can work. (I think). Because we are facing problem only in sets (in the array). Lemme try this once. I think I can get it done.

[MonkeyDo]

We ARE sorting it by ids. https://github.com/bookbrainz/bookbrainz-data-js/blob/1b3bf5c12326a8419401f283e25e014aca917347/src/util.js#L158

how about we compare relationshipSet and IdentifierSet by brute force. Picking up one relationship from the old set and check if it's present in the new set? Because we are facing problem only in sets (in the array). Lemme try this once. I think I can get it done.

Hmmmm. It's been a while I looked into this in detail :p
I wonder then if on the contrary you should try not sorting the array and letting the new library handle it.
That being said, I don't know if we can assume the sets will be returned in the same order every time by the ORM…

[MonkeyDo]

Looking at the sorting code again, it looks like we're not sorting by ID but rather by value and type.label, which might explain why it's all over the place.
You could try sorting by id or by type.id, that might do the trick.

[prabalsingh24]

@MonkeyDo I'll be afk for about a week. I'll continue this later. Is that alright?

[MonkeyDo]

@MonkeyDo I'll be afk for about a week. I'll continue this later. Is that alright?

Of course! You certainly do not need my permission !
Take care!

[MonkeyDo]

This looks much better already!
The issue with the diff library regarding arrays of objects (like identifiers) will be for another day (I created this separate issue).

However I'm seeing some issues with the relationship formatting:

I'm not exactly sure why that's happening, but on the Author side, the relationship that appears is not the correct one: "Compiler -> da17a8fa-…" (an edition) instead of "Author -> 8ee4cace-…".
It looks fine on the Work side however, and definitely an improvement over what was there XD.

[MonkeyDo]

You can Ignore the previous review.
The rel formatting issue I was seeing was due to my local copy of bookbrainz-data that I was doing experiments with, didn't catch that at the time.
As for the suggestion to fetch 'data.aliasSet.defaultAlias', that doesn't seem to work either.