Link to other action versions #395

alarthast · 2025-02-26T17:26:38Z

Addresses #27

There is a now a section showing all versions of the action, with the version for the current page highlighted, and other versions as clickable links
To implement the above, versions are ordered by their committed_at timestamp in the database

Further implementation details are in the commits.

Something to consider before merging this PR

Since we define the latest version as the one with the latest committed_at, the latest version of deciles charts is v0.0.43.
There is however a version 0.0.44, which has a committed_at that is 1 minute earlier than the one for 0.0.43.
Merging this PR will lead to a bubble for v0.0.44 showing up on the page for deciles charts, after v0.0.43 (see screenshot below). I think this is okay, but it's just something that we should be aware of.

iaindillingham

Thanks for working on this issue. It's great to see that you've got stuck in, and that the codebase was legible. I have a few comments about the structure of the PR; and a few comments about an alternative implementation.

First, the structure of the PR. Have you come across "How to Write a Git Commit Message"? You've followed six of the seven "rules", but not quite the seventh.

I often step through the commits that comprise a PR one by one. I got as far as the first commit and wanted to know why you were refactoring a test module and what you'd done, exactly. It took me a while to realise that you'd added a fixture that creates and returns an instance of Action; and also that you'd added a function that creates and returns an instance of Version. Why? What should I expect when I step through to the second commit?

I'm reasonably sure you were trying to reduce the amount of code in the test module, which is laudable. However, I feel that create_version is a code smell. If Version is inextricably linked to Action, then shouldn't we be creating and associating these objects somewhere else in the codebase? Somewhere other than in the test module? Do we need a new method on Action? A separate module for commands? For more, see Martin Fowler's "CQRS" article. However, for the moment, I don't think we need to worry about reducing the amount of code.

Having stepped through to the second commit, I'm unclear what I should have expected. I'm reasonably sure that create_version means that I'm not looking at a call to Version.objects.create, but again, I'm unclear.

The third commit is straightforward enough, but remember those "rules". Could the commit message be improved, at all?

Second, an alternative implementation. We're going to have to order versions at the application layer, rather than at the database layer. I'm afraid I disagree with you: v0.0.44 appearing before v0.0.43 isn't okay. (Thank you, though, for highlighting this in the PR description!) I'd suggest adding a method like Action.get_versions(), which would return a list of versions in the semantically correct order. Come to think of it, we could use a custom manager, although that might be going too far. Either way, we're going to have to write the code for determining the semantically correct order, and for handling errors gracefully. Let me know if you'd like to pair!

tests/actions/test_models.py

actions/models.py

tests/actions/test_models.py

iaindillingham · 2025-02-27T14:43:18Z

actions/templates/actions/version.html

+            {% if action_version.tag == version.tag %}
+              <span class="inline-flex items-center px-2.5 py-0.5 rounded-full text-xs font-medium uppercase bg-oxford-100 text-oxford-600">
+                {{ action_version.tag }}
+              </span>&nbsp;
+            {% else %}


Would it be such a bad thing to include the link in all cases? It would reduce the amount of logic in our template and I don't think anyone would mind if the target of the link was the current page.

I've chosen here to distinguish the version on the present page and other versions using colours (oxford blue vs grey). If we keep that, there still needs to be if/else logic in the template to switch between CSS styles.

While I don't think anyone would mind having a link, when I put my site-visitor hat on, my brain definitely finds it more satisfying that the current version is not clickable.

I'm sure it does, but I'm questioning whether it does enough to warrant the extra code.

I have revised the template and used a more "symmetric" structure for the if/else block, where it's hopefully clear that the colours are the only difference.

Thank you 🙂

- This is done to improve readability of the tests, since we anticipate more unit tests will be added with subsequent changes to the `Action` and `Version` models. - Move the creation of a dummy `Action` object to a pytest fixture, and reuse it to eliminate duplicate code - Remove unnecessary assignment of names to dummy `Version` objects by using `Version.objects.create`

- We will be making a change to show all past versions of an action to the user. - To display the action versions from latest to oldest, a sort is needed either database-side or application-side. - As of current plans, we would only retrieve versions from the database to get the latest version or display them in reverse- chronological order. - Given that a retrieval is always coupled to a sort, we can neatly specify the ordering in the `Meta` class of the `Version` model. - A corresponding database migration is done, and a unit test is added to ensure that the commit timestamp is used for ordering.

- The "All versions" section is added so that users have a quick way of accessing past versions of an action. - The version being shown is highlighted in oxford blue, the rest are in grey - Clicking on any version bubble sends the user to the corresponding page - There are tailwind classes in the <a> element rather than <span>, as this makes the whole bubble clickable instead of just the text

alarthast · 2025-03-03T12:52:50Z

Following the PR review there was a video call discussion on how to proceed. It turns out that due to the use of annotated tags, concurrent builds might lead to version tags being out of step with the chronological order of the commit timestamps.

In the above deciles chart case, the latest version is v0.0.43, not 0.0.44 (See opensafely-actions/deciles-charts#113), and that's the version we should suggest to the user by default. The decision is to continue using the committed_at timestamp to sort the versions.

I have tried improving the commit messages and making the code changes less messy.

iaindillingham

Thank you for your work on this PR. The structure is much clearer. And I withdraw my comment about the alternative implementation, because the bug lies elsewhere (opensafely-actions/deciles-charts#113). I've made a couple of comments about the tests that I'd like you to consider. If you have any questions, then please ask.

iaindillingham · 2025-03-04T11:30:25Z

tests/actions/test_models.py

    # Create a version and check get_latest_version() get this
-    version = Version(
+    Version.objects.create(


This is nice. I'd not realised that we were assigning to version because we needed to call .save.

actions/models.py

iaindillingham · 2025-03-04T12:04:57Z

actions/models.py

@@ -16,7 +16,7 @@ class Meta:
    def get_latest_version(self):
        """Return version with latest committed_at."""

-        latest_version = Version.objects.filter(action=self.id).latest("committed_at")
+        latest_version = Version.objects.filter(action=self.id)[0]


It's my conviction that software development is a craft. So, I'm going to suggest that we don't index into the queryset, but instead we use .first. I'm not aware that there's a performance penalty either way; but even if there was, then it wouldn't matter because my sense is that .first is easier to understand than [0].

iaindillingham · 2025-03-04T12:07:13Z

tests/actions/test_models.py

+    )
+    versions = action.versions.all()
+
+    assert len(versions) == 3


I don't think we need this assert. If it wasn't the case that there were three instances of Version, then we'd need to ask Django why not.

iaindillingham · 2025-03-04T12:11:33Z

tests/actions/test_models.py

@@ -14,6 +14,28 @@ def action():
    )


+def test_versions_are_ordered_from_latest_to_oldest(action):


Let's use the language that Django uses. So, either first/last or latest/earliest. Alternatively, let's use newest/oldest.

Suggested change

def test_versions_are_ordered_from_latest_to_oldest(action):

def test_versions_are_ordered_from_latest_to_earliest(action):

iaindillingham · 2025-03-04T12:13:13Z

tests/actions/test_models.py

+    versions = action.versions.all()
+
+    assert len(versions) == 3
+    assert versions[0].tag == "v1.1"


Again, my sense is that .first is easier to understand.

iaindillingham · 2025-03-04T12:24:09Z

tests/actions/test_models.py

+    Version.objects.create(
+        action=action,
+        tag="v1.0",
+        committed_at=datetime.now(timezone.utc),
+    )
+    Version.objects.create(
+        action=action,
+        tag="v1.2",
+        committed_at=datetime.now(timezone.utc),
+    )
+    Version.objects.create(
+        action=action,
+        tag="v1.1",
+        committed_at=datetime.now(timezone.utc),
+    )


Let's specify our datetime.datetimes explicitly. And let's not create test data that reflects a specific bug in a specific reusable action (opensafely-actions/deciles-charts#113). We would expect the "committed at" ordering to be the same as the "tag" ordering. Both will ensure that when we read this test in the future, we won't be confused.

You might find "Write Tests for People" by Gerard Meszaros useful and interesting.

iaindillingham · 2025-03-04T12:26:34Z

actions/templates/actions/version.html

+            {% if action_version.tag == version.tag %}
+              <span class="inline-flex items-center px-2.5 py-0.5 rounded-full text-xs font-medium uppercase bg-oxford-100 text-oxford-600">
+                {{ action_version.tag }}
+              </span>&nbsp;
+            {% else %}


Thank you 🙂

alarthast linked an issue Feb 26, 2025 that may be closed by this pull request

Link to other action versions #27

Open

iaindillingham reviewed Feb 27, 2025

View reviewed changes

alarthast added 3 commits March 3, 2025 12:45

alarthast force-pushed the aw/show-versions branch from 6f8dced to 3a2ff2b Compare March 3, 2025 12:46

alarthast requested a review from iaindillingham March 3, 2025 12:52

iaindillingham mentioned this pull request Mar 3, 2025

Race condition opensafely-actions/deciles-charts#113

Closed

iaindillingham reviewed Mar 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Link to other action versions #395

Link to other action versions #395

alarthast commented Feb 26, 2025

iaindillingham left a comment

iaindillingham Feb 27, 2025

alarthast Feb 27, 2025

iaindillingham Feb 27, 2025

alarthast Mar 3, 2025

iaindillingham Mar 4, 2025

alarthast commented Mar 3, 2025 •

edited

Loading

iaindillingham left a comment

iaindillingham Mar 4, 2025

iaindillingham Mar 4, 2025

iaindillingham Mar 4, 2025

iaindillingham Mar 4, 2025

iaindillingham Mar 4, 2025

iaindillingham Mar 4, 2025

iaindillingham Mar 4, 2025

iaindillingham Mar 4, 2025

		@@ -14,6 +14,28 @@ def action():
		)


		def test_versions_are_ordered_from_latest_to_oldest(action):

Link to other action versions #395

Are you sure you want to change the base?

Link to other action versions #395

Conversation

alarthast commented Feb 26, 2025

iaindillingham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alarthast commented Mar 3, 2025 • edited Loading

iaindillingham left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alarthast commented Mar 3, 2025 •

edited

Loading