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: feature flag consumption #4489

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

Conversation

mcgarrye
Copy link
Collaborator

@mcgarrye mcgarrye commented Nov 20, 2024

This PR addresses #(4459)

  • Addresses the issue in full
  • Addresses only certain aspects of the issue

Description

Creates the module, controller and service files for the new Feature Flag table. Allows for standard CRUD methods, as well as a list endpoint and an endpoint to add/remove jurisdiction associations with the flag (associateJurisdictions). Permissions are limited to admin only. Flags associated with jurisdictions will appear when jurisdiction data is returned.

How Can This Be Tested/Reviewed?

Start up the backend. Login as an admin, either through the API or partners portal. Test each endpoint:

  • Create a feature flag
  • Get the feature flag using the id returned ^^
  • Update the feature flag using the id
  • Get the feature flag to verify update
  • Associate jurisdiction(s) with the flag
  • Get the feature flag to verify association
  • Get the jurisdiction to verify association
  • Call the list endpoint
  • Remove the jurisdiction(s) using associateJurisdictions
  • Get the feature flag to verify removal
  • Get the jurisdiction to verify removal
  • Delete feature flag
  • Call list endpoint to verify

Author Checklist:

  • Added QA notes to the issue with applicable URLs
  • Reviewed in a desktop view
  • Reviewed in a mobile view
  • Reviewed considering accessibility
  • Added tests covering the changes
  • Made corresponding changes to the documentation
  • Ran yarn generate:client and/or created a migration when required

Review Process:

  • Read and understand the issue
  • Ensure the author has added QA notes
  • Review the code itself from a style point of view
  • Pull the changes down locally and test that the acceptance criteria is met
  • Either (1) explicitly ask a clarifying question, (2) request changes, or (3) approve the PR, even if there are very small remaining changes, if you don't need to re-review after the updates

@mcgarrye mcgarrye added the wip This PR is not ready for review, do not review it's a “Work In Progress” label Nov 20, 2024
Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for partners-bloom-dev ready!

Name Link
🔨 Latest commit 3bdd7e2
🔍 Latest deploy log https://app.netlify.com/sites/partners-bloom-dev/deploys/673f9e6cf222740008ac73aa
😎 Deploy Preview https://deploy-preview-4489--partners-bloom-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Nov 20, 2024

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit 3bdd7e2
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/673f9e6c8a5f430008711e01
😎 Deploy Preview https://deploy-preview-4489--bloom-exygy-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mcgarrye mcgarrye added 2 reviews needed Requires 2 more review before ready to merge and removed wip This PR is not ready for review, do not review it's a “Work In Progress” labels Nov 21, 2024
@mcgarrye mcgarrye marked this pull request as ready for review November 21, 2024 15:32
Copy link
Collaborator

@ludtkemorgan ludtkemorgan left a comment

Choose a reason for hiding this comment

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

Tested out all of the endpoints and they work great! Just the one question about if we can change the type on the associate endpoint. But otherwise good, so approving!

);
}

@Get(`:featureFlagId`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't have an explicit reason for it yet, but at some point we might want a get endpoint that takes in the "name" of the feature flag since we won't know the id. But that doesn't need to be done with this PR.

@Type(() => IdDTO)
@IsArray({ groups: [ValidationsGroupsEnum.default] })
@ApiProperty({ type: IdDTO, isArray: true })
associate: IdDTO[];
Copy link
Collaborator

Choose a reason for hiding this comment

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

By having this be an IdDTO the api shows that it wants an "ordinal"

{
  "id": "string",
  "associate": [
    {
      "id": "string",
      "name": "string",
      "ordinal": 0
    }
  ],
  "remove": [
    {
      "id": "string",
      "name": "string",
      "ordinal": 0
    }
  ]
}

Would this work if the type is string[] instead?

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 wanted the validations on the id field which would be missing with a string[] being passed in. This is the patten I could find elsewhere

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the validation just that it is a UUID? Are you not able to do the validation on string because it's an array?

@ludtkemorgan ludtkemorgan added 1 review needed Requires 1 more review before ready to merge and removed 2 reviews needed Requires 2 more review before ready to merge labels Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 review needed Requires 1 more review before ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants