Skip to content

Conversation

@AWitcherONS
Copy link
Contributor

@AWitcherONS AWitcherONS commented Jun 27, 2025

What is the context of this PR?

Bundles User Interface Journey from creation to Approval

Testing Strategy
User/Auth
Bundle:- volume, and contents
The feature tests should be completed in a timely manner avoiding duplication where possible

role, function (Search(volume is significant), Create, Edit/insert Edit/remove, send to moderation, preview, approve)
for each of these function what text can be seen, what fields to edit(insert/delete), what actions (save etc)
what happens when mandatory fields fail validations on actions

User role Bundle Life Cycle Happy Path 1 bundle Authorisation summary
                             | Bundles Search | Dashboard Search | Create | Edit | Preview | Approve |
    Publishing Admin         | Can            |                  | Can    | Can  | Can     | Can     !
        As Creator of Bundle | N/A            | N/A              | N/A    | N/A  | N/A     | Cannot  |
        Not Creator          | N/A            | N/A              | N/A    | N/A  | N/A     | Can     |
    Publishing Officer       | Can            | N/A              | Can    | Can  | Can     | N/A     |
        As Creator of Bundle | N/A            | N/A              | N/A    | N/A  | N/A     | Cannot  |
        Not Creator          | N/A            | N/A              | N/A    | N/A  | N/A     | Can     |
    Viewer                   | N/A            | N/A              | Cannot | N/A  | N/A     | N/A     |
        not in preview team  | Cannot         | N/A              | N/A    | N/A  | Cannot  | N/A     |
        in preview team      | Can            | N/A              | N/A    | N/A  | Can     | N/A     |

/

How to review

The Python code should comply with current standards
The feature tests run without failures and do not impact on run time

Follow-up Actions

A Viewer cannot use the Search in Bundles menu there isn't an error but no know bundles are not found
review on approval Authorisation of bundles

@zerolab zerolab changed the title Feature/bundles UI test cms 333 Bundles UI tests Jul 2, 2025
mark_page_as_ready_to_publish(release_calendar_page,user)
release_calendar_page.save_revision().publish()
context.release_calendar_page = release_calendar_page
print(context.release_calendar_page.pk)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leftover print statement

Comment on lines 311 to 312
Title = "Bundles UI Testing"
Summary = f"{Title} Summary"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should never capitalise the first letter of a variable name in Python. This is reserved for class names, and can create a lot of confusion.

Please read through this to understand Python naming conventions: https://peps.python.org/pep-0008/#prescriptive-naming-conventions

context.page.get_by_role("button", name="Publish").click()
context.article_path.append(Title)

def get_url_for_article_edition(context):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't seem to be using this anymore?

context.page.get_by_role("button", name="Save draft").click()
context.page.get_by_role("button", name="More actions").click()
context.page.get_by_role("button", name="Publish").click()
context.article_path.append(Title)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is article_path used for?

context.page.get_by_label("Page explorer").get_by_role("link", name="Bundles UI Testing 2", exact=True).click()
context.page.get_by_role("row", name="Select Articles More options").get_by_label("Select").check()
context.page.get_by_role("link", name="Add a child page to 'Articles'").click()
Title ="Bundles UI Article Series Page"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here, wrong naming convention

Comment on lines 174 to 177
# | 1 | Publishing Admin | Publishing Officer | {"role": "Publishing Admin", "creator_role": "Publishing Officer", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} |
# | 1 | Publishing Admin | Publishing Admin | {"role": "Publishing Admin", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} |
# | 1 | Publishing Officer | Publishing Officer | {"role": "Publishing Officer", "creator_role": "Publishing Officer", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} |
# | 1 | Publishing Officer | Publishing Admin | {"role": "Publishing Officer", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a reason why you have this commented out, it would be prudent to either leave a comment explaining why, or update the PR description so that this is clear.

Same on line 62, 63 and 211.

if bool(bundle_dets["add_stat_page"]) and hasattr(context, "statistical_article_page"):
page = context.statistical_article_page
BundlePage.objects.create(parent=bundle, page=page)
# context.bundlepages.append(BundlePageFactory(parent=bundle, page=page))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code left here.



@then("the {role} can preview the Release Calendar page")
def preview_release_calendar_page(context: Context, role: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to highlight a thing picked up by the linter: you have an unused argument here (role). We've discussed this already that you can word your step differently to avoid the need of mentioning the role - this will be implied based on previous steps.

  functional_tests/features/bundle_ui.feature:63  A User cannot save a bundle due to duplicate schedule -- @1.1 bundles
  functional_tests/features/bundle_ui.feature:64  A User cannot save a bundle due to duplicate schedule -- @1.2 bundles
  functional_tests/features/bundle_ui.feature:172  A user can approve a bundle -- @1.1 bundles
  functional_tests/features/bundle_ui.feature:173  A user can approve a bundle -- @1.2 bundles
  functional_tests/features/bundle_ui.feature:174  A user can approve a bundle -- @1.3 bundles
  functional_tests/features/bundle_ui.feature:175  A user can approve a bundle -- @1.4 bundles
  functional_tests/features/bundle_ui.feature:208  A user cannot approve a bundle due to lack of pages or datasets -- @1.1 bundles
Copy link
Contributor

@MaciekBaron MaciekBaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that in your latest commit you included a list of failing tests. They are failing because there are mistakes in your code. I have added a few comments to help you fix one of them ("A user can approve a bundle"). It's possible that these changes will also fix your other tests. They may also get resolved once you fix the things I mentioned in my other comments.

Comment on lines +414 to +416
mark_page_as_ready_to_publish(release_calendar_page, user)
release_calendar_page.save_revision().publish()
context.release_calendar_page = release_calendar_page
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your test is failing because you are creating a new revision and publishing it after you have marked it as ready to publish. This resets it state.

There's no reason for you to do that anyway. Just remove that line to fix this.

Suggested change
mark_page_as_ready_to_publish(release_calendar_page, user)
release_calendar_page.save_revision().publish()
context.release_calendar_page = release_calendar_page
mark_page_as_ready_to_publish(release_calendar_page, user)
context.release_calendar_page = release_calendar_page

Comment on lines 471 to 472
bundle.release_calendar_page = context.release_calendar_page
BundlePage.objects.create(parent=bundle, page=page)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next, your test will fail because you are assigning a release calendar page to the bundle and then adding it to the bundle as a bundled page. This is not allowed, and the validation will pick this up.

@then("the logged in user can approve a bundle")
def can_approve_bundle(context: Context) -> None:
bundle_edit = f"Edit '{context.bundles[-1].name}'"
bundle_status = f"Bundle '{context.bundles[-1].name}' Approval' updated. Edit"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't correct and fails the test - the Approval' bit is unnecessary

Comment on lines 92 to 114
Scenario Outline: A User can edit a bundle
Given there is a <role> user
And there is a <creator_role> user
And there is a Statistical Analysis page approved by <creator_role>
And there is a Release Calendar page approved by <creator_role>
And there is a preview team
And the <role> is a member of the preview team
And there are <number_of_bundles> bundles with <bundle_details>
When the <role> logs in
Then the logged in user can find the bundle
And the logged in user goes to edit bundle
And the logged in user can add a release schedule
And the logged in user can add pages
And the logged in user can add preview team
And the logged in user can send bundle to moderation


Examples: bundles
| number_of_bundles | role | creator_role | bundle_details |
| 2 | Publishing Admin | Publishing Admin | {"role": "Publishing Admin", "creator_role": "Publishing Admin", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} |
| 1 | Publishing Admin | Publishing Officer | {"role": "Publishing Admin", "creator_role": "Publishing Officer", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} |
| 11 | Publishing Officer | Publishing Admin | {"role": "Publishing Officer", "creator_role": "Publishing Admin", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} |
| 1 | Publishing Officer | Publishing Officer | {"role": "Publishing Officer", "creator_role": "Publishing Officer", "status": "Draft", "preview_teams": false, "add_rel_cal": false, "add_stat_page": false} |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have this test twice (line 69).



#---- Bundle Cannot preview UI Tests -----
Scenario Outline: A User cannot preview a bundle due to not member of associated preview-team
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Scenario Outline: A User cannot preview a bundle due to not member of associated preview-team
Scenario Outline: A User cannot preview a bundle due to not being a member of an associated preview-team

And there are <number_of_bundles> bundles with <bundle_details>
When the <role> logs in
Then the logged in user can approve a bundle
And the bundle pages are not live
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pages not being live isn't part of the scenario outline. Create a separate scenario if this is what you want to test - having said that, the ticket doesn't list this as a requirement.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it didn't make sense has been remove
resolved 6c68056ac

Comment on lines 180 to 195
Scenario Outline: A user cannot approve a bundle due to authorisation
Given there is a <role> user
And there is a <creator_role> user
And there is a Statistical Analysis page approved by <creator_role>
And there is a Release Calendar page approved by <creator_role>
And there is a preview team
And the <role> is a member of the preview team
And there are <number_of_bundles> bundles with <bundle_details>
When the <role> logs in
Then the logged in user cannot approve a bundle

Examples: bundles
| number_of_bundles | role | creator_role | bundle_details |
| 1 | Viewer | Publishing Admin | {"role": "Viewer", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": false, "add_rel_cal": true, "add_stat_page": true}|
| 1 | Publishing Officer | Publishing Officer | {"role": "Publishing Officer", "creator_role": "Publishing Officer", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} |
| 1 | Publishing Admin | Publishing Admin | {"role": "Publishing Admin", "creator_role": "Publishing Admin", "status": "In_Review", "preview_teams": true, "add_rel_cal": true, "add_stat_page": true} |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This scenario is not very clear.

You should have separate tests: one for a viewer not being to approve bundles, and then another one for creator of the bundle not being able to approve their own bundle. This makes it clearer and more maintainable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Test has now been removed as is no longer applicable
resolved 6c68056ac

Comment on lines 24 to 30
Scenario Outline: A User cannot create a bundle due to authorisation
Given there is a <role> user
When the <role> logs in
Then the logged in user cannot create a bundle
Examples: bundles
| role |
| Viewer |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's pointless to use a scenario outline if you only have one example. Convert this to a regular scenario to improve readability. Here and on lines 141 and 197.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your comment but these are the exceptions to the success scenarios above
my logic/readability is that the exception should replicate the success scenario to the point of failure....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Ann, I don't understand your reply.

Scenario Outlines are used for parameterised scenarios, where the scenario is executed for each row in the examples table.

However, you only have one example. It's a lot more readable to do it this way:

Suggested change
Scenario Outline: A User cannot create a bundle due to authorisation
Given there is a <role> user
When the <role> logs in
Then the logged in user cannot create a bundle
Examples: bundles
| role |
| Viewer |
Scenario: A User cannot create a bundle due to authorisation
Given there is a Viewer user
When the Viewer logs in
Then the logged in user cannot create a bundle

It's clear what we are testing, who the user is and what they are trying to do, and we use fewer lines. It all reads better – you don't need to go to the table to check what the <role> parameter will be. There is no reason to "replicate" the table from the scenario outline which has multiple different roles – I see no benefit in doing that. It just makes the code clunkier and more difficult to read. If you can show me a benefit, I can definitely take it into consideration.


@then("the logged in user cannot approve a bundle due to lack of pages")
def cannot_approve_bundle_edge_case(context: Context) -> None:
bundle_edit = f"Edit '{context.bundles[-1].name}'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This test fails here as it cannot find the edit link.
  2. This step should be split into at least two or three steps, e.g. "the user goes to the bundle view", "the user clicks approve", "the user is informed they cannot approve the bundle"
  3. You should check that the message "Cannot approve the bundle without any pages or datasets" is visible, not just that the page could not be saved, in case something else has prevented it from being saved - be explicit in your tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved in 612ac1c

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Point 2 hasn't been addressed.

Consider your current scenario ("A user cannot approve a bundle due to lack of pages or datasets"). After your initial setup with the Given + And statements, you have essentially just two steps:

    When the <role> logs in
    Then the logged in user cannot approve a bundle due to lack of pages

This is a functional/UI test. We should be explicit in what the user is trying to do. Look at existing tests in the bundle.feature file, like this example:

Scenario: A content editor cannot set a release calendar page to cancelled when it is in a bundle
When the user manually creates a future release calendar page with a "Provisional" status
And the user clicks the "Save Draft" button
And the user navigates to the bundle creation page
And the user enters a title
And the user opens the release calendar page chooser
And the user selects the existing release calendar page
And the user clicks "Save as draft"
And the user tries to set the release calendar page status to "Cancelled"
Then the user sees a validation error preventing the cancellation because the page is in a bundle

Notice how we clearly outline all of the steps the user is taking: what they are clicking, what they are putting in etc.. Your scenario doesn't do that. Some of your other scenarios suffer from the same problem.

Copy link
Contributor

@MaciekBaron MaciekBaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Ann for replying to some of my comments. It makes it easier to review and verify that you have addressed the issues raised. Could you please continue replying to all of the comments so that we know that everything has been addressed.

Comment on lines 618 to 641
@then("the logged in user adds a Name to the bundle")
def add_name_to_bundle(context: Context) -> None:
bundle_name = "Bundle UI Test Bundle"
if "bundles" in context:
bundle_name = context.bundles[-1].name
context.page.get_by_role("textbox", name="Name*").fill(bundle_name)


@then("the logged in user adds a Release Calendar page to the bundle")
def add_release_calendar_to_bundle(context: Context) -> None:
context.page.get_by_role("button", name="Choose Release Calendar page").click()
context.page.get_by_role("textbox", name="Search term").fill(context.release_calendar_page.title)
context.page.get_by_role("row", name=context.release_calendar_page.title).get_by_role("link").click()


@then("the logged in user add a schedule date to the bundle")
def add_schedule_date(context: Context) -> None:
now = (datetime.now() + timedelta(hours=4)).strftime("%Y-%m-%d %H:%M")
context.page.get_by_role("textbox", name="or Publication date").fill(now)


@then("the logged in user goes to the bundle page")
def go_to_bundle_page(context: Context) -> None:
context.page.get_by_role("link", name="Bundles", exact=True).click()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these shouldn't be @then statements but either @when or @step statements.

Please review all of your other statements because a lot of them suffer from the same issue. See my other comment which mentions the Gherkin documentation.

Additionally, there's a small typo on line 633 (should be "adds" not "add).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by
ba796fb

Comment on lines 124 to 127
When the <role> logs in
Then the logged in user goes to the bundle page
And the logged in user can find the bundle
And the <role> can preview bundle
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not using When/Then correctly.

When specifies the action performed by a user that triggers the behaviour to be tested

Then specifies the outcome or expected result of the action

Please consult the Gherkin documentation to get a better understanding of how to structure your tests and what these terms mean.

This mistake is repeated in your other tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved in 8ae5c18

Comment on lines 464 to 465
context.page.get_by_role("textbox", name="Search term").fill(context.bundles[-1].name)
context.page.goto(context.base_url + "/admin/bundle/?q=" + context.bundles[-1].name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't make sense. You fill in the search term and then force the browser to go to that URL.

You should instead be clicking the search button so that the search gets triggered. You make the same mistake on line 572.

Moreover - and I've highlighted this several times now in other places - you do not make any assertions here.

If this step is supposed to check that "the logged in user can find the bundle", what exactly are you doing to check that? You essentially just go to that URL and that's it. The URL could be wrong, it could return an error - many things can happen, but you're not checking for anything here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved by 79810cc
I agree this is confusing
its was 'clingon' from playwright codegen
it should be the success search results expects
I need to make this viewer friendly

@then("the logged in user can create a bundle")
def add_bundle_details(context: Context) -> None:
expect(context.page.get_by_role("link", name="Add bundle")).to_be_visible()
context.page.get_by_role("link", name="Add bundle").click()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear why do you click on the button here. You already checked it is visible.

Copy link
Contributor Author

@AWitcherONS AWitcherONS Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved by
ba796fb
it has now been split down to two steps
My logic ....
If a 'viewer' ran the success scenario it would fail at this point and is necessary to justify the exception

@MaciekBaron
Copy link
Contributor

@AWitcherONS in case you are wondering why your tests are failing, we have upgraded behave to the latest version and it is case sensitive when it comes to step definitions. As you are mixing cases, it fails to find them.

Please ensure consistent casing and to run the tests locally before pushing.

@AWitcherONS
Copy link
Contributor Author

Resolved by
ba796fb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants