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

Create incidents endpoint and tests #355

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

hrauniya
Copy link
Collaborator

Fixed endpoint and tests for creating new incidents.

@hrauniya hrauniya requested a review from DMalone87 March 10, 2024 04:11
Copy link
Collaborator

@DMalone87 DMalone87 left a comment

Choose a reason for hiding this comment

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

A few issues here:

  1. The test issue reference below.
  2. With the current rules for publishing, we need to make sure that a user is turned into a contributor once they become an admin or a publisher for a partner. (for example, here: https://github.com/hrauniya/police-data-trust/blob/d03729941831d9111a22c5e59d249133948107ae/backend/routes/partners.py#L65)
    There's a global User Role Contributor that the User needs to have.
    class UserRole(str, Enum):

@@ -53,18 +55,25 @@


@pytest.fixture
def example_incidents(db_session, client, contributor_access_token):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think contributor_access_token is fine here. We want to test the lowest allowable auth (and highest non-allowed auth) to make sure we get the permission decision right.

Copy link
Collaborator Author

@hrauniya hrauniya Mar 16, 2024

Choose a reason for hiding this comment

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

For issue 2, added the code to make sure that a user is turned into a contributor once they become an admin or a publisher for a partner. This is possible through two endpoints AFAIK, create_partners, and role_change.

Since create_partners has another PR open right now, I can add the updated tests through that PR and tests for it as well.

permission = PartnerMember.query.filter(
PartnerMember.user_id == user_id,
PartnerMember.role.in_((MemberRole.PUBLISHER, MemberRole.ADMIN)),
).first()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something to consider: we should make sure that the source included request body is the same source that is being used to publish the incident. It's may seem strange, but it could be that the Partners endpoint is actually the best place for the incident creation function.

Just opining though. No changes needed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried to move the create_incidents to routes/partners.py , but seems like it'll break other tests in test_incidents as it is needed to create mock incidents for testing other endpoints. I could figure out a work around to this, but wasn't sure if I should do it.

@hrauniya hrauniya requested a review from DMalone87 March 16, 2024 03:30
hrauniya and others added 18 commits March 15, 2024 23:31
…#349)

* Install necessary dependencies for API integration:

@tanstack/react-query
@tanstack/react-query-devtools
react-hot-toast (for displaying results of API calls)

* Add React Query/react-hot-toast setup in app providers

* Update test snapshots & jest config

Add dependencies to `esmNodeModules` for Jest to transform
Update snapshots to include react-hot-toast toast wrapper div

* Update @TanStack dependencies (to 4.36.1)
* Officer Table Overhaul (In Progress)
- Adding tests for Officer API endpoints
- Creating association objects for:
    - Accusation (Officer/Perpetrator)
    - Employment (Officer/Agency)

* Officer Model Updates

* Add additional officer endpoints and tests
- Get officer by id
- Get all officers
- Create officer

* Style Corrections

* Officer search error fix
* Fixes a warning about
`Attorney.legal_case_plaintiff` and
`Attorney.legal_case_defendant` being created improperly.

* Convert relationships to strings in the
legal_case model.

* Set User email column to `cache_ok` = true

* This time, with feeling...
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.

5 participants