BiologicalRecordsCentre / NPMS

NPMS
0 stars 0 forks source link

Change Squares Taken On/Released report to be more robust against corrupt accounts #330

Closed andrewvanbreda closed 2 months ago

andrewvanbreda commented 2 months ago

Currently if an account is corrupt, it does not show on the report. It would also be useful to mark corruptions in red.

andrewvanbreda commented 2 months ago

Note there is a problem with person_attribute_value allocation 59744 which has been created and updated by different people the very same second (possibly because of masquerading) and is being rejected by the report.

andrewvanbreda commented 2 months ago

Hi @sacrevert @Sam-Amy @NPMSSupport ,

I have looked at the way this report works.

I think actually it is quite difficult to get this report right. For instance, if an allocation is marked as deleted in the database, it doesn't mean that should be excluded from the taken on in period report, it just means it was taken on and then released. It also means the approval date is overwritten with the deletion date. So the data in the same place in the database can actually mean different things in different situation.

For now, what I have done is removed the logic that tries to get rid of useless rows, as actually I think it is excluding rows where an approval is made quickly after the request. However it does mean more mistakes will show on the report. In particular you may find there are now a few rows included where allocations are done for testing purposes such as by myself, also rows for people who took on a square and removed it but it was never approved. Those will now show on the report as a square taken on and approved, but the approval date will be wrong, and will actually be the date it was deleted.

I guess though the these problem rows will be heavily outnumbered by extra good rows you get to see now.

It is clear the report needs some further tweaking, I will try my best with that. One obvious thing I can do is supply a parameter to the report of accounts to ignore such as myself.

I will also do the suggested change where corrupt accounts are list in red, although I do not expect that to affect many rows. I have not done that yet.

Andy

sacrevert commented 2 months ago

thanks @andrewvanbreda I forget whether you were actively working on other NPMS issues, but, if not, then I think this one is now the priority

andrewvanbreda commented 2 months ago

@sacrevert Just to note, I am working on several things at once at sec on NPMS/PP other projects, but I will add this to priority list. The next thing which is now very close to go live is the mandatory abudance detection on Plant Portal standard mode.

andrewvanbreda commented 2 months ago

Hi @sacrevert @NPMSSupport @Sam-Amy ,

Please check the new version of the report on this page:

https://avb-report-brc-npms-d10.pantheonsite.io/squares-taken-on-released-in-period

Improvements

Just a general note on using the report (and this is the way it has always worked). On the grids, each time a square is attached to a user, it is displayed on a separate row. So if a square is attached to a user, then released, then reattached even to same user, it will show as two rows on the top grid, and one on the Release grid.

And another note about about corrupt accounts. This seems to be more common than I expected going by the red on the grids (although seems to be more on the release grid). John has fixed this issue, but am realising I will need to check other reports do not make the assumption the account isn't corrupt. I have already check the All Visits page, and it does make that assumption, so visits made by people with a corrupt account won't show. I will raise that separately.

sacrevert commented 2 months ago

I would be happy to have this moved to live, as, even if other minor improvements could be made, it seems like this work already represents significant useful work. Feel free to put on live and close this issue when you like.

andrewvanbreda commented 2 months ago

This is live now