Fixes #1817 : updated Notes and Temporary_Label_Id columns

[rpechuk]

Fixed #1817 by adding label type to the notes column and made sure that the temporary_label_Id was not being set to NULL

[misaugstad]

@rpechuk please link to the issue that is being fixed/resolves with "Resolves #" or "Fixes #" in the PR description.

[rpechuk]

@misaugstad I thought I did weird

[misaugstad]

OH I see where the confusion has been. Because I've asked you about this 2 or 3 times now, and I'm realizing that when I say "PR description", you're changing the title. I'm talking about the description, not the title

[rpechuk]

I'm not sure I can, cause I realize you meant the description but I don't have an option to change that anywhere

[misaugstad]

So it looks like a byproduct of your change is that the temporary_label_id is still being included in all the interactions after deleting the label. The issue before was that we stopped using the temp label id a little bit too early, not we are stopping way too late!

[rpechuk]

Hmmmm, ok I'll look into it, not sure what's causing that.

[misaugstad]

So I looked at the change you made in the code, and that is what I expected the correct fix to be. However, it looks like things are a bit more complicated than that! I tried deleting a label using a few different methods:

1. Place the label and then delete it before the context menu ever closes
2. Place the label, close the context menu, then delete the label with the context menu still closed
3. Place the label, close the context menu, reopen the context menu, then delete the label while the context menu is still open

I ended up getting a bunch of different incorrect behaviors. I think it did work sometimes, but it also is not working most of the time. Sometimes RemoveLabel doesn't have a temporary_label_id and sometimes Click_LabelDelete doesn't have a temporary_label_id.

One additional thing: if it is easy enough, can you add the label type to the Click_LabelDelete actions as well?

Here are some screenshots of the what the weird behavior looks like...

1. Click_LabelDelete is missing a temporary_label_id. I don't really care whether or not the mouseup event has it, but the Click_LabelDelete should.
   
2. And in this one, RemoveLabel is missing the temporary_label_id
   
3. And in this one, RemoveLabel is also missing the temporary_label_id
   

So I would just test more thoroughly in some different scenarios to make sure the solution is robust.

And I understand that this might be pretty difficult, and this issue is not important enough to an entire week on 😁 so I wouldn't worry too much about.

[rpechuk]

No worries i'm getting to know the logging so I think that this is a good issue to do that with. I did do quite a bit of testing and in my testing everything was fine did everything you mentioned but the third point. I'm gonna continue looking into it periodically maybe get some new insight into it. The whole table is huge and in my opinion could be improved so idk this could be a good first step. I can add the type of label to Click Label Delete action quite easily so will do.

[rpechuk]

Interesting so i've been looking into this issue a lot today and the thing that is causing all of these issues is:

if(self._isContextMenuClose(action) || self._isDeleteLabelAction(action) || !contextMenuLabel){
console.log(action);
currentLabel = null;
}

The reason for this is that the action order is the context menu closes before the ClickLabel_delete button action is logged and therefore the temp label id is being set to NULL.

You can see this here:

The second line shows that the code is in the code snippet I added above and of course after that line the temp label id will be null so the next one is null but the way we have it set is that the removeLabel action gets the labelID again above in the code so it has it again but then after this we can see that it is going back into the code snippet I have above and therefore the temp label id gets set to null once again. This is super weird and i'm gonna continue looking into it.

[rpechuk]

What I did in order to fix the issue is add a new function that checks if the action is the Click Label Delete action and changed it so that when the action is a Click Label Delete action the "Current Label" gets updated with the correct temp label id and then gets set to null after being deleted.

[misaugstad]

cool so is this ready for testing and review again @rpechuk ?

[rpechuk]

@misaugstad Yep :)

[misaugstad]

On this branch, whenever I try to delete a label, I'm getting the following error:

[misaugstad]

Oops, didn't mean to send yet! Here's what I was going for...

On this branch, whenever I try to delete a label, I'm getting the following error:

Uncaught ReferenceError: label is not defined
    at HTMLImageElement.labelDeleteIconClick

The offending line seems to be:

svl.tracker.push('Click_LabelDelete', {labelType: label.getProperty('labelType')});

[misaugstad]

This seems to be working great @rpechuk nice work! Just need to fix a typo in the name of a function 👍

[rpechuk]

Fixed sorry for the delay, I never got a notification even though i'm signed up for them.

[misaugstad]

Great, thanks!