Track history of user-made changes

mrmin123 commented 9 years ago

Currently we're relying on the owner (soon to be submitted_by) and dateTime (soon to be date_created) fields to track user-made changes at the moment, which is a transient and incomplete history at best. We should incorporate a way to to track most (if not all) user-made changes for a more accurate 'Recent History' on the Dashboard and (more importantly) auditing and tracking purposes.

forresttanaka commented 8 years ago

Proposal

Each DB-changing action creates a user history object — a new object requiring a new schema.

The User History object contains:

User @id
Operation enum (“add,” “modify,” “delete”) :triangular_flag_on_post:
@id of affected primary object (like subject of sentence)
@id of affected secondary object (like direct object of sentence)
@id of associated object (like the indirect object of a sentence)
Time of operation in ISO 8601 format
Text description of action for human readability, with embedded codes for variable replacement.

Mechanism

Each action requires writing a new user history object to the DB. Assuming all actions involve either writing new objects to the DB or updating old ones, we can tie this mechanism to the routines that all of ClinGen uses to write or update objects:

rest.putRestData: Write a single new object to the database
rest.postRestData: Update a single object in the database
rest.postRestDatas: Update an array of objects in the database

All these eventually call rest.writeRestData. I propose having this function write a user history object to the database, as code doesn’t have to be duplicated, and rest.writeRestData has error handling built in so it can easily make writing the user history object conditional on a successful database write.

Object deletion also needs to wind up in the user history. As we don’t yet have this ability, I’ll pretend a rest.deleteRestData method exists that takes the URI of the object to be deleted, and returns a promise that would probably get ignored in most cases. This method writes an entry to the user history object :triangular_flag_on_post:

Operations That Result in a New User History Object (and suggested messages)

In the list below, “-1” after the name shows a primary object, a “-2” shows a secondary object, and “-3” shows the associated object.

AssessmentMixin.saveAssessment: Used when saving any kind of assessment. The assessment object would be primary, but it has no human-readable identifying information, so we don’t display anything for it.
- For pathogenicity
  Assessed variant clinvarVariantId-2 pathogenicity.
- For experimental:
  Assessed experimental data experimental.label-2.
- For segregation:
  Assessed family family.label-2 segregation.
CurationCentral.updateGdmArticles: Used when adding a new PubMed article to a GDM. “GDM ID” means the spelled-out group, disease, and mode.
- PMID:article.pmid-1 added to GDM ID-2.
CurationMixin.updateOmimId: Used when updating the OMIM ID of a GDM.
- GDM ID-1 OMIM ID changed to gdm.omimId.
Group.submitForm: Used when creating or editing a group.
- Group.label-1 added to GDM ID-2 for PMID:article.pmid-3.
- Group.label-1 modified.
FamilyCuration.writeFamilyObj: Used when creating or editing a family. Other objects can get modified when creating a family (e.g. Group), but those don’t get included in the history.
- Family.label-1 added to GDM ID-2 for PMID:article.pmid-3.
- Family.label-1 added to Group.label-2 for PMID:article.pmid-3.
- Family.label-1 modified.
IndividualCuration.writeFamilyObj: Used when creating or editing an individual. Other objects can get modified when creating an individual (e.g. Group), but those don’t get included in the history.
- Individual.label-1 added to GDM ID-2 for PMID:article.pmid-3.
- Individual.label-1 added to Group.label-2 for PMID:article.pmid-3.
- Individual.label-1 added to Family.label-2 for PMID:article.pmid-3.
- Individual.label-1 modified.
ExperimentalCuration.submitForm: Used when creating or editing experimental data. Other objects can get modified when creating an individual (e.g. Annotations). Extra spaces between the parens exist because of markdown limitations — they wouldn’t exist on the site..
- Experiment.label-1 ( Experiment.evidenceType ) added to GDM ID-2.
ProvisionalCuration.submitForm: Used when creating or editing a provisional classification.
- Provisional classification added to GDM ID-2**.
- Provisional classfication for GDM ID-2** modified.
VariantCuration.submitForm: Used when adding a pathogenicity to a variant. Pathogenicities have no human-readable identifying information, so it doesn’t get displayed here.
- Variant clinvarVariantId-2 curation added.
- Variant clinvarVariantId-2 curation modified.
Because we don’t yet have any user operations that can delete objects nor any Javascript functions to do this, I’ll just give a generality here. Generally, the user history message will have the form: :triangular_flag_on_post:

Deleted object-1 from object-2.

Notes :triangular_flag_on_post:

I could see a mechanism to generate standard sentences for display using a combination of the objects and the Operation enum, but I’d worry this could generate awkward or not very informative sentences, and feel each operation generating a fleshed-out sentence makes a better user experience.
The Operation enum could be used to filter entries, but most of its use would probably come from adding a corresponding color and/or icon to each kind of operation display.

mrmin123 commented 8 years ago

Two additional suggestions:

An additional field in the User History object that denotes what type of operation it was (Add, Edit, Delete)
Messages for Delete operations

kilodalton commented 8 years ago

@forresttanaka @mrmin123 Thanks for the time and thought spent on elaborating a detailed spec for this feature that we discussed in our Thursday meeting. It will help us preserve historical provenance especially in the context of deletions, make user and item history more transparent and easier to access especially with multiple curators, and be able to track site metrics more easily. Nice job.

forresttanaka commented 8 years ago

@mrmin123 Good stuff!

forresttanaka commented 8 years ago

Updated the proposal above to add the operation type and notes about deleting objects. I also added a couple of notes at the bottom. Any changes have a little triangular flag marking them.

mrmin123 commented 8 years ago

@forresttanaka regarding your notes, I agree with you on not using generated sentences. If generated, whatever interface we have to show these would have to do a query on the references objects whenever it wants to get a name or label. And yea, the reason I suggested the operation enum was mainly for filtering, but it would be nice to have a color/icon indicators for all the actions, especially for something like an All History view.

forresttanaka commented 8 years ago

Wanted to make sure this was possible before going any further: can one property of an object embed more than one type of object, i.e. could the “primary,” “secondary,” and “associated” properties each embed the different kind of affected objects? Yes they can as the attached image shows. So code reading the history object can then use the embedded objects’ @type to figure out what kind of object it is.

[Edit: To be clear, I don’t think we’re likely to initially use the “primary,” “secondary,” and “associated” properties. It simply seems dangerous not to include a reference to the very objects affected in case we find some use for them in the future. Initially though, we’ll embed links (like /curation-central/?gdm=xxx) in the history object, and directives in the string say where to put the links]

screen shot 2015-11-09 at 15 30 56

mrmin123 commented 8 years ago

My initial thinking was that embedding the objects would be overkill, but perhaps there are some use cases for it that I'm not thinking of right now. @forresttanaka if you can potentially see a need for it in the future, should we just go with embedded objects to begin with? Changing it in the future once in production seems like it'd be a real pain.

forresttanaka commented 8 years ago

@mrmin123 I’ll plan on including the embedded objects for now. Then maybe once things work, we can set up some performance comparisons with and without embedding. It would probably make no significant difference when writing the objects to the DB, but I could imagine some difference when reading in an object the back end has to put together.

kilodalton commented 8 years ago

It's possible that the size of all returned objects with full embedding of all related objects could be huge, and might be a performance problem for the dashboard view for users with a lot of activity that needs to be queried and displayed on the dashboard.

forresttanaka commented 8 years ago

The nice thing is that you can choose how deep to embed the objects, so really we might only include the top-level objects, none of which are all that large. If they include embedded arrays of objects it can get large, but we can simply choose not to embed those. But I could see the argument “If you’re not embedding the objects within the objects, why include the objects at all if you’re not using them?” The answer right now is just that it makes me feel better. But it may well not have any value.

At this stage I’m thinking I could put them in the schema and simply not use them. That way if we find a use in the future, it would involve a Javascript change, but no schema change.

forresttanaka commented 8 years ago

As a progress report, I can now save history items to the DB and then use a single routine to render it with links. The string that generates the screen shot below contains:

Group {P:GRP4} added to {S:DICER1-Achondroplasia Autosomal dominant inheritance (HP:0000006)} for {A:PMID:123}

The problem of course is the italics and boldface. I’m not really sure yet the best way to encode those in the string, or to do things a completely different way. I thought about a really pared down Markdown syntax, but even really pared down, the processing might be kind of laborious.

Anyway, here’s how it looks when creating a group:

screen shot 2015-11-11 at 16 28 59

forresttanaka commented 8 years ago

…and here’s how the history object looks in the DB. Codes in the description property specify where the primary, secondary, and associated properties go, and what their link text says. the elements property has corresponding objects containing the link text and a reference to the relevant objects themselves, which I don’t use and don’t get embedded.

screen shot 2015-11-11 at 16 44 07

forresttanaka commented 8 years ago

Solved the styling problem by changing the history objects pretty much completely. Each operation being done now not only controls writing data to the history object, it also handles displaying the data. For example, groups not only control the writing of their data to the DB, a function belonging to the group module now gets called by the dashboard through the same registration mechanism we use to display pages. The dashboard only knows it’s calling whatever owns the “primary” object (still there), and that primary object’s code then knows how to display its own data with whatever styling it wants.

The bottom entry of the attached image shows the new display mechanism. The top three are the existing dashboard display mechanism.

screen shot 2015-11-12 at 15 27 45

forresttanaka commented 8 years ago

Lots of testing needed, but here’s the dashboard displaying history objects. Lots of code that retrieved and interpreted each object removed.

history

[Edit: Oops — “experimental data segregation”?]

forresttanaka commented 8 years ago

Give it a try. It should have no visual impact except on the dashboard’s “Your Recent History” section.

~~https://215-user-history-34b0bf4-fytanaka.demo.clinicalgenome.org~~

selinad commented 8 years ago

Looking great @forresttanaka - wow - it is capturing a lot of actions!

A few comments:

have variant ID go to Edit Variant page
when submitting experimental and it is assessed by curator, the assess history should be chronologically after the submission (ie. on top of it in the history)
Could the new OMIM ID be hyperlinked on history page when changed (not a big deal).
When name of an individual is changed, it gives same message as it gives when it is added the first time: " Individual ind1.1 added to DICER1-Pleuropulmonary blastoma-Autosomal dominant inheritance with maternal imprinting for PMID:7847369; 2015 Nov 30, 5:54 pm" - not sure how to get around this unless "Individual ind1 updated ind1.1 in DICER1-Pleuropulmonary blastoma-Autosomal dominant inheritance with maternal imprinting for PMID:7847369; 2015 Nov 30, 5:54 pm" (where ind1.1 is hyperlinked?)
Sometimes the verb is at the front "Assessed..." and sometimes the at end "Variant pathogenicity added" - seems like it would be better to be consistent throughout. If you could churn out a list of all of your phrases, I could take a look.
Is it supposed to say when it has saved a Provisional Classification? That does not seem to have happened...
As for the other assessment, would be slightly better if the assessment occurs chronologically after modifying the family, if that's possible and not too difficult.

screen shot 2015-11-30 at 6 05 02 pm

forresttanaka commented 8 years ago

Good stuff @selinad! I’ll go over these tomorrow. Offhand, I don’t see a problem with any of these, though the last one will be a bit clunky to implement. The order displayed is actually the order it happens, so some information will have to skip past others. Should be doable though.

selinad commented 8 years ago

Great - thanks @forresttanaka ! Perhaps I did something funny with my order. I think it should do it in align with my actions, as you have it, so let's leave it. I do think I assessed last on the Experimental, though, and the order was different ...

Let me know if I missed any actions....thx!

forresttanaka commented 8 years ago

@selinad No, that’s just the way it works — the assessment gets recorded, then the thing it assesses gets recorded. I agree it looks awkward. If you create something, then assess it later, then it shows in the natural order. It’s just when you create and assess on the same page and save them together, it gets recorded in the current order.

forresttanaka commented 8 years ago

…so I’ll work on flipping that order in the history tomorrow.

selinad commented 8 years ago

Got it. In that case, flipping would be good!

forresttanaka commented 8 years ago

@selinad

Sometimes the verb is at the front "Assessed..." and sometimes the at end "Variant pathogenicity added" - seems like it would be better to be consistent throughout.

These are two different actions though. One is assessing a pathogenicity, and the other is adding a variant pathogenicity regardless of the assessment.

screen shot 2015-12-02 at 11 14 51

selinad commented 8 years ago

Could it be:

Variant 1000 pathogenicity assessed as Supports Variant 1000 pathogenicity information added

OR

Assessed variant 1000 pathogenicity to Supports Added pathogenicity information to variant 1000

forresttanaka commented 8 years ago

Here it is. I’ll spin up an instance shortly

screen shot 2015-12-02 at 12 26 09

forresttanaka commented 8 years ago

Hopefully addresses all of @selinad bullet points:

~~https://215-user-history-5e2150d-fytanaka.demo.clinicalgenome.org/~~

selinad commented 8 years ago

Thanks @forresttanaka - your screen capture looks good - will take a look at instance.

selinad commented 8 years ago

Hi @forresttanaka - great!

A few things:

Would it be difficult to add the Classification level in? e.g. "Provisional classification LIMITED added to DICER1-Pleuropulmonary blastoma-Autosomal dominant inheritance with maternal imprinting"
How difficult to add that a variant has been added to the record? (could be done at a later date)
It would be cool to put a link for the record's history up in that 3rd column of the header (where Status is) - if people think this is a good idea, I could make a ticket for it for a future release....would be nice go from there to history instead of having to go to Dashboard, but this history would be record dependent.

One kinda strange thing:

I added Group G1, then went to home - did not show G1
I added Group G2, then went to home - showed G1
Refreshed page and G2 was there as well (in history)

selinad commented 8 years ago

p.s. I like the way you changed around the wording - make it easier on the eyes as well since hyperlinks are in more uniform position (generally)

forresttanaka commented 8 years ago

OK, there’s a much larger problem: it shows everyone’s recent history as your history. So I’ll fix that too.

forresttanaka commented 8 years ago

@selinad or @kgliu0101

Would it be difficult to add the Classification level in? e.g. "Provisional classification LIMITED added to DICER1-Pleuropulmonary blastoma-Autosomal dominant inheritance with maternal imprinting"

I see autoClassification and alteredClassification. Which one should I show?

kgliu0101 commented 8 years ago

If necessary, alteredClassification is the one. In default, alteredClassification = autoClassification. It may be changed by user manually.

forresttanaka commented 8 years ago

One thing I don’t link to in the history list are provisional classifications. I’m not sure what to link to, nor what text in the history the link should surround. Suggestions? Should a link exist for these at all?

screen shot 2015-12-03 at 09 19 19