BB-394 fix(display): Deleted work not shown in the edition anymore.

[panwarabhishek345]

Problem

BB-394 Deleted Works still appear on Edition page

Solution

We check if the work.typeId is null or not before displaying the WorkTableRow. TypeId is always going to be not null if the work has not been deleted yet.

Areas of Impact

src/client/components/pages/entities/work-table.js

[coveralls]

Coverage remained the same at 42.982% when pulling 80ddb6a on panwarabhishek345:deleted-work-in-edition into 40314ad on bookbrainz:master.

[MonkeyDo]

Hi ! Thanks for your PR !

This is a reasonable PR, the logic is exactly what I would expect, and it does solve the issue on a surface level.

However it addresses the symptom rather than the cause: we shouldn't have deleted entities in that list.
It's likely that this affects the (in construction) API too, so solving at the root will be beneficial for multiple projects.

So you'll have to follow the data up the to find out how that list of Works is created, and apply the same filter (no dataId = deleted, that is exactly how to do it) at the source.

This PR remains valid, though, so you can continue working on the same.

[MonkeyDo]

So in short, when a user deletes an entity, any entity it has a relationship with is not updated to remove said relationship.

So if I add a relationship Edition contains Work, that creates two relationships: #1 on the Edition's RelationshipSet and #2 on the Work's RelationshipSet.
If I deleted the Work, #2 doesn't matter anymore, but #1 stays in the Edition's RelationshipSet. When we try to display that, we get that (unnamed) where the Work was.

That's the root cause of this issue: relationships aren't updated when deleting an entity.
That's a harder task, and I'm actually working on that mechanism at the moment (for the merging tool #333 where the same issue happens), so I can update you when I have a working mechanism, and I can explain it to you so that you can integrate it in the entity deletion process.
Does that sound good?

[panwarabhishek345]

Yes, that sounds good.

[MonkeyDo]

The process was quite tricky.
Let's say there's an entity A with relationships to entity B and C.

If I delete A, you have to get the relationships of A to get linked entities' BBIDs (in this case the BBIDs for entities B and C).
Then, for B and C, you need to fetch that entity and its relationshipSet, then remove any relationship that has A's BBID in it, and save all that into a new revision.

Here's the code I used to achieve that in the merge tool: ae02baf#diff-0eaa45e02a38b45e1429cfb72db6b17eR650-R707
It's not the prettiest nor the most efficient, but it works.

Have a look, and let me know if you have any questions.
It's a bigger task than what you originally signed up for, and I hope you're up for the challenge :)
If not, don't worry, let me know and we'll figure it out.

[panwarabhishek345]

Yes, I'll look into it. If I have any doubts, I'll talk to you.

[MonkeyDo]

Hi @panwarabhishek345 !
Sorry for the delay, I've had to leave unexpectedly and be away from my computer.
I'll do my best to review this week, and in any case will be fully back after the 25th.

[MonkeyDo]

This isn't going to work (and neither is the new HeaderModel(… below).
The reason for that is RevisionModel and HeaderModel refer to a specific entity type.
For example, if I'm deleting an Author, it will be AuthorRevision and AuthorHeader.

Now if the Author I'm deleting has a relationship to a Work —for example— you're trying to create a new AuthorRevision and AuthorHeader with the Work's bbid (instead of a WorkRevision and WorkHeader).

That is currently throwing an error for me, and it's not what you want to do.
Instead, you would need to get the right model for the each updated entity.
The type (Author, Work…) you can get with entityModel.get('type'), and then use the utils.getEntityModelByType utility to fetch the right model.

However, I think we can get away with not using the entity revision and header at all, and instead use the saveEntitiesAndFinishRevision utility that is used when editing entitites (have a look in the same file at how it is used in handleCreateOrEditEntity).
It also takes care of setting the parent revision ids, incrementing the editor count and creating a revision note, so it seems like a good idea to use that same utility instead of repeating code (a good refactor for free :) )

You'll have to try that out and see how it works out.
I know it's quite a heavy part of the website's mechanism, but you're doing a great job so far !
Good luck with the continuation :)

[panwarabhishek345]

@MonkeyDo Sorry for the late response. I would be able to resume my work from the 29th due to some work I am caught up with.

Regarding the issue with PR, thanks for clearing that out. Also, I am tried using the utils functions but it didn't work out hence I rewrote the code just to check. All these commits I made just to let you know what is cooking in my mind regarding the PR and to show the progress. Its rough work and will make it fair once I resolve the issue.

Thanks

[MonkeyDo]

@MonkeyDo Sorry for the late response. I would be able to resume my work from the 29th due to some work I am caught up with.

Regarding the issue with PR, thanks for clearing that out. Also, I am tried using the utils functions but it didn't work out hence I rewrote the code just to check. All these commits I made just to let you know what is cooking in my mind regarding the PR and to show the progress. Its rough work and will make it fair once I resolve the issue.

Thanks

Oh, don't worry at all @panwarabhishek345, I trust you to have your priorities straight and I'm not expecting anything more than what —and when— you're able to work on it :)

It's a complicated part of the codebase, I do realise that, and it's bound to take some time even just to understand what's going on!

[prabalsingh24]

Hi, why did you replace promises with async-await?
I am also not super sure about this. @MonkeyDo correct me if I am wrong, but using promises will make your code run faster right? because await blocks the execution of the code and by doing that you're not using asynchronous nature of JavaScript to your benefit.

[panwarabhishek345]

@prabalsingh24 Async Await for Better readability. Apart from that I tried to keep code blocking as least as possible. But as I mentioned in the above comments. This is a rough code to just get the things working for now. Once things work, then I will think about the efficiency and readability.

[MonkeyDo]

Hi, why did you replace promises with async-await?
I am also not super sure about this. @MonkeyDo correct me if I am wrong, but using promises will make your code run faster right? because await blocks the execution of the code and by doing that you're not using asynchronous nature of JavaScript to your benefit.

@prabalsingh24
async/await is mostly just another way if writing promises for it to look like synchronous code. In fact, an async functions returns a Promise, just like creating a new Promise.
As for await blocking code execution, it's equivalent to having multiple .then() callbacks with a Promise-based approach: most of the time you want to run some asynchronous code and then do something synchronous with the result.

You can also await multiple async functions at the same time (making it non-blocking asynchronous code again) using await Promise.all(myArrayOfAsyncFunctions)

In the case of this PR, with a complex set of promises it becomes complicated to read, and even more complicated to react properly to any failure in the chain.
Moving to async/await code makes it a lot easier to read, extend, catch errors to make it more robust.

Here's a good run-down of some benefits of async/await: https://blog.pusher.com/promises-async-await/
It's not always appropriate, but when you find yourself in callback hell looking for who's calling chat and where does the result end up, the way out is async/await :)
(A good example of callback hell in here, not too far from the type of code we have: https://medium.com/better-programming/javascript-promises-and-why-async-await-wins-the-battle-4fc9d15d509f)

[panwarabhishek345]

I hope that is what you meant in the process you told me to implement in the code. I have deliberately avoided the use of map. Please read the comment in the code for an issue I am facing. Thanks

[MonkeyDo]

You're on the right track, this is almost working !

[MonkeyDo]

Here's what I did with that for the merging tool:
a448238#diff-0eaa45e02a38b45e1429cfb72db6b17eR709-R715 (lines 709-715)

The new RelationshipSet is created by the ORM helper function updateRelationshipSets (https://github.com/bookbrainz/bookbrainz-data-js/blob/1d36915dcd5b895c6860048d8051a79784618967/src/func/relationship.js#L88)

[MonkeyDo]

Hi @panwarabhishek345 !

Do you have time to have another stab at finishing this or would you like me to take it off your hands?

[panwarabhishek345]

Yes you can go ahead with this. I have been caught up with another project. Sorry for the delay. :)

…

On Sun 5 Apr, 2020, 6:01 PM Monkey Do, ***@***.***> wrote: Hi @panwarabhishek345 <https://github.com/panwarabhishek345> ! Do you have time to have another stab at finishing this or would you like me to take it off your hands? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#347 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFMHKOAUQT4DY6NE6UEWLKLRLB235ANCNFSM4KLVWVKA> .

[MonkeyDo]

Closed in favor of #412