BiologicalRecordsCentre / plantportal

Focused repo for the Plant Portal website
0 stars 0 forks source link

Allow import reversal #12

Open andrewvanbreda opened 3 years ago

andrewvanbreda commented 3 years ago

Imports should be reversible. Should be able to work with either the Plant Portal Import, or the general Indicia one. Can use the import-guid field to set the deleted flag on the imported rows.

andrewvanbreda commented 2 years ago

Hi @sacrevert

Are you wanting this included in the quote?

If so, I think we need to work out what it is actually going to do. Think if this is to be implemented this needs to be simple and not too ambitious.

This could become impractical to program if we aren't careful.

For instance (if it were to be a completely general importer): Things become very tricky if we need to reverse updated exsiting data rows as well as new rows (as we would need to keep track of what the data used to be). Not only that, this would need to work regardless of the type of data, and also that data might have been updated again before a reverse request comes in.

Also need to consider things like the permissions of who can reverse which imports, and the age of imports that allow reversal.

So I am not too hopeful of making a general Indicia reversal for all situations.

Even removal of new data isn't actually that straightforward. For instance, if a sample is created and that row has added a new plot/plot group, what happens if that sample is reversed, are they plot groups/plots also reversed? what if those are now in use by other samples.

Just off top of my head, the only thing I can think of that might not get too complicated. Limit to new rows only. Only the person who did the import can reverse it. Still need to consider what to do if a sample row has suseqently been updated before reversal is requested. Limit the list of available imports a user can reverse to ones within a certain age. Only reverse the samples, not the plot groups/plot (which could now be in use)

Although the more restrictions we place on the reverser, the less useful it is.

Are reversals actually likely to be that regular, I am almost thinking a manual SQL correction is easier if a user alerts administrators to a problem.

DavidRoy commented 2 years ago

@andrewvanbreda can you see what is offered by the new importer that John has been working. My understanding is that it loads the data into temporary tables, does some checks, reports back the user who can then either confirm or 'reverse' the import. Maybe fits this requirement?

Or otherwise, some functionality to delete based on an import-guid sounds like useful Indicia functionality. But only 'all or nothing' for an import

andrewvanbreda commented 2 years ago

Hi @DavidRoy Yes my notes are based after an email to John, then I looked into it myself. I have started looking at the new importer as the Plant Portal importer will be based on that (although at the moment am just preparing a quote for Oil, so my experience is still very limited with the new importer).

I suppose it depends what you mean by reversal. The reversal you talk about there is essentially a check before the import is finally committed to the database, then deciding not to proceed, rather than a full reverse after an actual import is done.

I had assume Oli meant the latter. @sacrevert perhaps you can comment on what you need?

I am not sure how long the temp table is available before they are removed, but that would severely limit the timescale with which a reversal could be done if we relied on those.

So I guess I come back to my original thought, that we need to lock down the specification for something useful but that is still programmable.

John had suggested perhaps making use of the audit module, but I think that is problematic as I think it is highly configurable what is audited, and there are lots of problems with reversing updated rows.

sacrevert commented 2 years ago

Given that the importer will already do various error checks before upload is allowed, and that (I presume) users can delete plots, samples, and occurrences that they own, perhaps we have no need for this. Please close if you agree. Thanks

DavidRoy commented 2 years ago

I think we could simplify this to:

but @sacrevert to confirm

andrewvanbreda commented 2 years ago

@sacrevert @DavidRoy OK, not sure whether this is to be closed or investigated. What I will do is have a look tomorrow, and discuss with John about a deletion just defined by guid, then we can decide whether to close or proceed.

sacrevert commented 2 years ago

@andrewvanbreda please investigate what David said to see whether it is achievable easily. Thanks

andrewvanbreda commented 2 years ago

Hi @DavidRoy @sacrevert ,

I did speak to John about this yesterday. This is what we think.

Allow reversal of new Samples/Occurrences rows from an import.

but it won't reverse

Things that will be reversed

This should keep things nice and simple, and keep it reliable. We don't want something complicated that doesn't work properly.

Let me know if you have any questions.

sacrevert commented 2 years ago

Thanks @andrewvanbreda . If you can quote on that basis, that would be good.

andrewvanbreda commented 6 months ago

Update on this issue as requested by Oli

Coding is partially complete.

Once it is finished, the import reversal will work with importer V2 for some imports. One's that include updates to existing data will either not reverse, or will only be reversible partially (if they also include import of new data also).

Imports that are too old also probably won't reverse, although I am not quite sure how old "old" is yet.

I think import v2 is currently used on iRecord and will include the new function. All places that use the V2 importer will have the option to use it. I will speak to John about whether it should only be enabled when the page is configured to show it. Plant Portal's sample occurrence importer will include the reverse, and the location (e.g. square) imports might include the reversal if they are switched over to V2, I have not checked if they are suitable for that yet.