alan-turing-institute / AssurancePlatform

Project to facilitate creation of Assurance Cases
MIT License
21 stars 6 forks source link

[User Story]: Trigger identifier renaming manually #483

Closed cptanalatriste closed 2 months ago

cptanalatriste commented 3 months ago

Role

As an Assurance Case Designer

Desired Feature

I want to trigger identifier manually, instead of automatically

Benefit

So that I can take actions before renaming happens.

Acceptance Criteria

GIVEN that I've added removed case items AND their names are inconsistent WHEN I click the "Reset Names" button THEN the names of Context, Strategies, Properties, and Evidence are consecutive and sequential.

Dependencies

No response

Technical Notes

Development Tasks

Definition of Done

cptanalatriste commented 3 months ago

The endpoint is complete (see branch attached to issue). @RichGriff will work on the GUI changes

RichGriff commented 3 months ago

Frontend action button toolbar has been updated to include the button for 'Reset Names' - this call the endpoint and reload the assurance case with a page reload.

@cptanalatriste PR ready

RichGriff commented 3 months ago

In staging - @chrisdburr please review

chrisdburr commented 3 months ago

Thanks, @RichGriff .

One immediate change is the text for the tooltip that appears on hover. Please can we change the text from 'Reset Names' to 'Reset Identifiers'.

And can we have a modal/alert that pops up saying the following:

Are you sure? Updating the identifiers will systematically reset all of the unique labels that are displayed for each of the elements (e.g. P1, E1), so that they are continuous. This cannot be undone.

No, keep current identifiers. Yes, reset all identifiers.

chrisdburr commented 3 months ago

@cptanalatriste, please could you add information about what is actually happening when a user clicks this button so I can review thoroughly.

RichGriff commented 3 months ago

@chrisdburr - The frontend change is now in staging

chrisdburr commented 3 months ago

Thanks, @RichGriff. I can confirm that the button label and modal text is correct. However, I'm still not clear on what is happening when this action is actually triggered.

For instance, here is a new case that I created from scratch. Each element is numbered (description not identifier) according to the order in which I created them. However, for some reason the property claims are not numbered incrementally in the identifiers.

Image

I then modify the case by adding some new elements, deleting others, and moving some around. This updates the identifiers for the property claims but not the evidence:

Image

Finally, I trigger the renaming of the identifiers, which returns this:

Image

This seems to take care of the duplicate identifiers, and it appears as though the numbering is now based on the order in which the elements are created. @cptanalatriste please can you explain if this is the expected behaviour?

My preference would be for the identifiers to be continuous from left-to-right when the user tiggers a refresh. So, the following:

cptanalatriste commented 2 months ago

@chrisdburr I think this is aligned with the implementation currently in staging: Top-level property claims are sorted by insertion order (i.e ,the one created first will be P1, the one created last will be P6).

Sadly, the backend is unaware of how/where the case elements are placed in the screen. Given that we are triggering a page reload on identifier update, as a first approximation I would propose a brand-new ordering of case elements after refresh (i.e. some elements might be placed in new positions, but respecting the sequencing logic you mentioned before). Although I would expect this change to take 2-3 days of work, given the new case traversal logic needed.

Also, just to understand the new rules better, can you include the expected Identifiers for the last screenshot you included?

cptanalatriste commented 2 months ago

@chrisdburr after our discussions, I made an update to the "Update Identifiers" algorithms (see PR https://github.com/alan-turing-institute/AssurancePlatform/pull/540 ): It is now "parent aware". With the new version, a case with this configuration (notice the "order" in description if you want to replicate this):

after_fix

Have the following configuration after update:

after_fix

These changes are already in staging. Please give it a shot, and let me know if there's another "blind spot" it is urgent to address.

chrisdburr commented 2 months ago

This is looking good, @cptanalatriste.

I haven't been able to break it yet, which is promising. So, I'd be happy to merge this, because if there are any further issues we can just log the individual bugs.

Well done!