alan-turing-institute / AssurancePlatform

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

[BUG] Identifier numbering is not clear #489

Closed chrisdburr closed 1 week ago

chrisdburr commented 2 weeks ago

Issue

The numbering for the element identifiers does not appear to follow a consistent logic.

Screenshot 2024-06-19 at 09 46 48

As you can see in this image, the child elements of S1, which were the first to be added start at P6.

While not shown on this image, P1...P5 also exist and are located under S4.

For some reason, the strategy elements also appear to have moved around (as shown below). Their numbering indicates the order in which they were created, but this is not reflected in their left-to-right ordering.

Screenshot 2024-06-19 at 09 55 53

Desired Behaviour

Sequential numbering of identifiers should be consistent, and ideally based on the order in which elements are added.

chrisdburr commented 2 weeks ago

Another example. The following strategy elements were added in this order:

Initially, the numbering remained consistent until Argument over fair impacts was added. Then, the numbering was automatically revised.

Screenshot 2024-06-19 at 10 57 56
cptanalatriste commented 2 weeks ago

@chrisdburr just identified the root cause and prepared a PR with the fix: https://github.com/alan-turing-institute/AssurancePlatform/pull/496

I'm deploying the fix to staging right now. Please verify if now it's behaving as expected.

chrisdburr commented 2 weeks ago

Initial testing on my end seems to have fixed the bug. Please could I request some more thorough testing on this though. @kallewesterling do you have suggestions for a more systematic way of testing this?

cptanalatriste commented 2 weeks ago

@chrisdburr due to the root cause (a random ordering on rows when performing the query) testing this one manually it's tricky. I can write an automated unit test, so it can flag if the issue reappears.

chrisdburr commented 2 weeks ago

Thanks, @cptanalatriste. Whatever method you think is best I am happy to go with.

cptanalatriste commented 2 weeks ago

Please verify in staging.

chrisdburr commented 2 weeks ago

This seems to be working. However, we will need to update this when we add the feature to refresh identifiers across the case.

Please link to a PR when closing.

cptanalatriste commented 1 week ago

The original fix was part of PR: https://github.com/alan-turing-institute/AssurancePlatform/pull/496/files

The automated test was added in PR: https://github.com/alan-turing-institute/AssurancePlatform/pull/508/files

Closing as agreed with @chrisdburr