Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IA-2986: Have "sub org unit types to display" by default #1346

Merged
merged 7 commits into from
Jun 13, 2024

Conversation

butofleury
Copy link
Member

@butofleury butofleury commented May 31, 2024

Related JIRA tickets : IA-2986

Self proofreading checklist

  • Did I use eslint and black formatters
  • Is my code clear enough and well documented
  • Are my typescript files well typed
  • New translations have been added or updated if new strings have been introduced in the frontend
  • My migrations file are included
  • Are there enough tests
  • Documentation has been included (for new feature)

Changes

Setting up default sub org unit type.

How to test

Launch setuper to generate data and check the org unit type page: http://localhost:8081/dashboard/orgunits/types/.

You should see sub org unit type.

Print screen / video

Once the test data created, check the org unit type page: http://localhost:8081/dashboard/orgunits/types/

Sub-Org-unit-type.mp4

@butofleury butofleury requested review from mestachs and kemar and removed request for mestachs May 31, 2024 11:43
self.assertHasField(response.json(), "sub_unit_types", list)
new_sub_units = response.json()["sub_unit_types"]
new_sub_unit_ids = [sub_unit_type["id"] for sub_unit_type in new_sub_units]
self.assertEqual(new_sub_unit_ids, sub_unit_type_ids)
Copy link
Member

Choose a reason for hiding this comment

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

Is this test supposed to cover setuper.pyramid.update_org_unit_sub_type?

I'm asking because the logic seems replicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, It's supposed to cover setuper.pyramid.update_org_unit_sub_type

Copy link
Member

Choose a reason for hiding this comment

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

Ok.

In fact, the test shouldn't re-implement the logic. Otherwise, if you change the function, the test will continue to work.

So, it would be better if setuper.pyramid.update_org_unit_sub_type could return something that you can test to ensure that it's doing what you want.

Copy link
Member

@kemar kemar left a comment

Choose a reason for hiding this comment

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

I have a question in comments.

@madewulf madewulf added the release Should be released in production at next deploy label Jun 3, 2024
@butofleury butofleury requested a review from kemar June 4, 2024 06:57
Copy link
Member

@kemar kemar left a comment

Choose a reason for hiding this comment

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

LGTM (the test could go one step further, it's up to you to decide)

updated_with_sub_types = update_org_unit_sub_type(self.client, self.project.id, response_data["orgUnitTypes"])

for org_unit_type_with in updated_with_sub_types:
self.assertJSONResponse(org_unit_type_with, 200)
Copy link
Member

Choose a reason for hiding this comment

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

Do we know the expected new sub types here?
If so we should test that too in addition to testing the returned HTTP code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am wondering for testing the expected new sub type, if from the line 27 we filter on the expected new sub types with ;

expected_new_sub_types = []
for org_unit_type in response_data["orgUnitTypes"]:
    org_unit_type_level = org_unit_type["depth"]
    sub_unit_type_ids = [
        org_unit_type["id"]
        for org_unit_type in response_data["orgUnitTypes"]
        if org_unit_type["depth"] == (org_unit_type_level + 1)
    ]
    expected_new_sub_types.append(sub_unit_type_ids)

But, it looks a re-implement of the logic of update_org_unit_sub_type

Then, from line 29

updated_sub_types = []
for org_unit_type_with in updated_with_sub_types:
    self.assertJSONResponse(org_unit_type_with["updated_org_unit_typs"], 200)
    updated_sub_types.append(org_unit_type_with["sub_unit_type_ids"][0])

sub_types_ids = reduce(lambda a, b: a + b, expected_new_sub_types)
self.assertEqual(updated_sub_types, sub_types_ids)

Copy link
Member

Choose a reason for hiding this comment

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

TBH I don't know if it's doable?

My point is that In general, we know what we are expecting from a test:

expected_result = "foo"

result = test_my_function()

assert result == expected_result

Here, the only thing that you are testing is that the API call returns 200, but we don't know if the data was modified in the expected way.

We can discuss this tomorrow, it may be easier to talk about it.

Copy link
Member

Choose a reason for hiding this comment

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

Are we really doing tests on the setuper? I'm thinking this might be overkill.

@kemar kemar removed the release Should be released in production at next deploy label Jun 4, 2024
@madewulf madewulf merged commit a01b2d6 into main Jun 13, 2024
3 checks passed
@madewulf madewulf deleted the IA-2986-Have_sub-org-unit-types-to-display-by-default branch June 13, 2024 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants