Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

BB-585: Revisions should clearly mark removed entities #626

Merged
merged 8 commits into from
Sep 1, 2021

Conversation

akashgp09
Copy link
Contributor

Problem

This PR Fixes: BB-585

Solution

  • utilized entity dataId to mark a deleted entity.
  • deleted entity should have parentAlias and existing entity should have defaultAlias

Before

Screenshot from 2021-05-08 23-05-10

After

Screenshot from 2021-05-08 20-27-38

Areas of Impact

Signed-off-by: Akash Gupta <akashgp9@gmail.com>
Signed-off-by: Akash Gupta <akashgp9@gmail.com>
Signed-off-by: Akash Gupta <akashgp9@gmail.com>
Signed-off-by: Akash Gupta <akashgp9@gmail.com>
@coveralls
Copy link

Coverage Status

Coverage increased (+0.009%) to 60.919% when pulling 50204d2 on akashgp09:mark-removed-entity-revision into 0aea892 on bookbrainz:master.

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

This works well for the most part. Seems to display deleted entities correctly in all cases I checked.

I'm seeing a regression in the way merged entities are displayed though, showing the BBID of the entity they are merged into rather than their own BBID :
Compare this screenshot from bb.org:
image

… to this one running this PR locally:
image

A clue might lie in this comment in the deleted code in diffFormatters/entity.js :
For entities without data (deleted or merged), we use getEntityParentAlias instead which returns a JSON object

Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

I think this is almost ready. Deleted entities are shown as expected, so that part seems solved.

However I believe that now a revision where we create a new entity will appear like the entity was deleted:

Screenshot from this revision mentioned in the BB-585 ticket: http://localhost:9099/revision/3212
image

src/server/routes/revision.js Show resolved Hide resolved
Clarify the meaning of the "new", "merged" and "deleted" badges in the revision page, as well as the hover text on strikethrough entity names (deleted entities)
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

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

Thanks for this good feature !
Now it will be a lot clearer when an entity has been deleted, even looking at its history.

I took the liberty to make the modifications I suggested on IRC; I added hover text for the strikethrough names and for the "new", "deleted" and "merged" badges to help avoid possible confusion.

Hovering on the strikethrough name:
image

Hovering on the "new" badge:
image

@MonkeyDo MonkeyDo merged commit b8ffe3b into metabrainz:master Sep 1, 2021
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