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

Feat: add integration test for the stakeholder #1774

Open
wants to merge 4 commits into
base: v2
Choose a base branch
from

Conversation

GuningShen
Copy link

@GuningShen GuningShen commented Mar 28, 2024

Description

comment: # I mock some data following the new v2/stakeholder/[stakeholderid] api for the cypress integration test.

Fixes #1769

Type of change

  • New feature (non-breaking change which adds functionality)

Screenshots

Before After
"screenshot before" Screenshot 2024-04-11 at 8 46 49 PM

How Has This Been Tested?

  • Cypress integration

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

method: 'get',
path: '/query/v2/stakeholder/1',
statusCode: 200,
body: stakeholder,
Copy link
Collaborator

Choose a reason for hiding this comment

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

The organization mock json is deprecated, please create a new fixture (under fixture folder) json file for stakeholder , the spec of stakeholder is defined in our query api doc here:
https://github.com/Greenstand/treetracker-query-api/blob/fa194cb4a274ee688c2b26b58596147368d08b9e/docs/api/spec/query-api.yaml#L1542-L1559

@dadiorchen
Copy link
Collaborator

@GuningShen your pull request looks pretty good, I added some comment:

  1. The mock data above
  2. Please fix the failure unit test, you can check the log by clicking the details

@GuningShen
Copy link
Author

GuningShen commented Apr 8, 2024

Hi, @dadiorchen thanks for commenting. I am quite confused looking at the stakeholder schema. It seems the new version does not contain fields like created_at, org_name, etc and I cannot figure out how to render the organization page without mocking those variables.
Screenshot 2024-04-08 at 11 32 47 AM

@dadiorchen
Copy link
Collaborator

@GuningShen sorry for the inconvenience, the final definition of stakeholder is here:

                          Table "stakeholder.stakeholder"
   Column   |           Type           | Collation | Nullable |      Default
------------+--------------------------+-----------+----------+--------------------
 id         | uuid                     |           | not null | uuid_generate_v4()
 type       | character varying        |           | not null |
 org_name   | character varying        |           |          |
 first_name | character varying        |           |          |
 last_name  | character varying        |           |          |
 email      | character varying        |           |          |
 phone      | character varying        |           |          |
 website    | character varying        |           |          |
 logo_url   | character varying        |           |          |
 map        | character varying        |           |          |
 created_at | timestamp with time zone |           | not null | now()
 updated_at | timestamp with time zone |           | not null | now()
 active     | boolean                  |           | not null | true
 entity_id  | integer                  |           |          |

The yaml file is out of date, so please mock the stakeholder based on this db structure.

@dadiorchen
Copy link
Collaborator

Aslo it would be great if you can raise a pr to update our yaml file in query API.

@GuningShen
Copy link
Author

GuningShen commented Apr 9, 2024

Aslo it would be great if you can raise a pr to update our yaml file in query API.

I just submitted a pr in query API! Please check that out. Thanks.

Copy link
Collaborator

@yunchipang yunchipang left a comment

Choose a reason for hiding this comment

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

overall lgtm! organization & stakeholder integration tests passed.
feel free to merge it after resolving the conflicts 🚀

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'd rename this fixture to be planter.json (I assume he's a planter)

cy.task('nockIntercept', {
hostname: 'https://dev-k8s.treetracker.org',
method: 'get',
path: '/query/v2/stakeholder/1',
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I would use

path: `/query/v2/stakeholder/${stakeholder.id}`,

instead of

path: '/query/v2/stakeholder/1',

to populate the stakeholder id in path dynamically. And this should apply to all other paths in this test file if you are modifying any of them.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @yunchipang , thanks for your suggestion! I've just changed it. I agree it will make my code look cleaner.

@dadiorchen
Copy link
Collaborator

@GuningShen sorry for the late review, could solve these conflict? we want to merge your pr

@GuningShen
Copy link
Author

@GuningShen sorry for the late review, could solve these conflict? we want to merge your pr

Hi @dadiorchen , I just resolved all the merge conflicts. Could you check whether my code looks good to you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants