-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix for categorical double save bug on annotation page #196
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for tracking this down @jarmoza! I think your changes should fix the bug (tested locally) 🎉 🐛 ! As you said, it would still be great to have a test. Both to make sure we understood the original problem, to check that this really fixes it, and so that it doesn't come back 🚫 .
One thing I also take away from this is that storing the "state" of mappings in the output table is probably more problematic than we initially thought. I.e.: the idea we discussed of only storing the actual mappings and then applying them only when the user is ready to download them is probably a much cleaner solution - and would have made fixing this easier as well. So, another strike in favour of rethinking the internal data model!
@jarmoza , didn't see your edit about this not being ready yet. You can also change the PR to a draft if you still want to work on it |
Didn't know about this functionality actually. Will utilize it next time I need it!. The code has been revised and component testing added into our Github action for this repo. Closing this out! |
@surchs Opening this up as the fix for bug #176 is ready in the related commit.
[Edit: I will be following this commit up with a component test for
annot-discrete-choices
, so hold up on this PR review until that commit is in.)There is also a question of establishing a UI paradigm regarding the 'Save Annotation' button. See below.
Fix description
The issue lay in how the
applyAnnotation
function inannot-discrete-choices.vue
sets new values. Once a new annotation value is saved it is set in the annotated table and we can no longer rely on the value in that table to help us find a newly set value in thevalueMapping
object. The latter is updated every time a new selection is made by the user (via theupdateMapping
function).So, what was needed is 1 (or possibly 2) additions to our code. The first is a utility getter function placed in the store
getOriginalColumnValue
that takes a subject ID and looks to get the original value of its column. This allows us to correctly find the key invalueMapping
for the newly-selected annotation value.The second piece is the ability to flag the enabled status of the 'Save Annotation' button as to whether or not a new annotation value has been entered into the interface. This is initially set to
false
on component created (seeinitializeMapping
) and is flagged totrue
onceupdateMapping
is called. Once a user has saved the annotation, the flag is once again set tofalse
. ThesaveButtonEnabled
value that is tied to the enabled status of the button makes use of this new flag.Let me know what you think of both pieces of the implementation.