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.

Sign up for GitHub

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

Jump to bottom

Hide forecasted grants while UI is being developed to support forecasted grants #3456

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

Hide forecasted grants while UI is being developed to support forecasted grants #3456

wants to merge 4 commits into from

Conversation

masimons
Copy link
Contributor

@masimons masimons commented Sep 2, 2024 •
edited
Loading

Ticket #3213

Description

Hide forecasted grants on production during development and testing.

Screenshots / Demo Video

Testing

Automated and Unit Tests

  • Added Unit tests

Manual tests for Reviewer

  • Added steps to test feature/functionality manually

Checklist

  • Provided ticket and description
  • Provided screenshots/demo
  • Provided testing information
  • Provided adequate test coverage for all new code
  • Added PR reviewers

@github-actions github-actions bot added the javascript Pull requests that update Javascript code label Sep 2, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2024 •
edited
Loading

QA Summary

QA Check Result
🌐 Client Tests ✅
🔗 Server Tests ✅
🤝 E2E Tests ✅
📏 ESLint ✅
🧹 TFLint ✅

Test Coverage

Coverage report for `packages/client`
St File % Stmts % Branch % Funcs % Lines Uncovered Line #s
🔴 All files 35.81 34.5 32.55 37.39
🔴  src 0 100 100 0
🔴   App.vue 0 100 100 0 2-9
🔴  src/arpa_reporter 0 100 100 0
🔴   App.vue 0 100 100 0 2-13
🟡  ...ter/components 58.58 48 46.8 59.78
🟡   AlertBox.vue 80 75 50 80 13
🔴   ...oadButton.vue 20 0 0 20 2-7,38-67
🟢   ...ileButton.vue 100 100 100 100
🟢   ...ttonSmall.vue 100 100 100 100
🟢   ...mplateBtn.vue 100 100 100 100
🟡   ...avigation.vue 67.74 63.63 52.63 67.74 ...13-219,228-235
🔴   StandardForm.vue 45 50 41.66 45.45 ...24-128,135-157
🟢  ...porter/helpers 84.61 79.48 87.5 84.61
🟢   form-helpers.js 84.21 79.48 85.71 84.21 7,16,25,81-83
🟢   short-uuid.js 100 100 100 100
🔴  ...eporter/router 0 0 0 0
🔴   index.js 0 0 0 0 20-131
🔴  ...reporter/store 3.92 0 2.17 4.12
🔴   index.js 3.92 0 2.17 4.12 11-14,32-261
🔴  ...reporter/views 40.98 25.13 41.37 42.91
🟢   AgenciesView.vue 100 0 100 100 16
🔴   AgencyView.vue 40.74 36.36 50 45.83 53-62,70-96
🔴   HomeView.vue 26.66 34.14 36.36 26.66 36-66,113,137-207
🔴   LoginView.vue 29.62 36.36 20 30.76 1,4,19-34,72-100
🔴   ...plateView.vue 27.02 35.71 45.45 25 ...2,30-37,69-113
🔴   ...ploadView.vue 24.24 16.66 33.33 26.66 1,30-31,116-144
🔴   ...eriodView.vue 48 18.18 75 52.17 64-90
🟡   ...riodsView.vue 57.57 28.57 60 59.37 101,132,149-171
🔴   ...pientView.vue 40 10.52 30.76 41.02 73-93,110-152
🔴   ...ientsView.vue 48.48 8.33 46.66 53.33 ...46,165-190,203
🟡   UploadView.vue 61.53 43.75 62.5 61.36 ...41-442,448-449
🔴   UploadsView.vue 16.66 0 0 18.18 59,110-287
🔴   UserView.vue 46.34 33.33 68.75 48.64 84,97-137
🟡   UsersView.vue 60 7.69 62.5 66.66 91-92,105-107,123
🔴   ...ationView.vue 37.03 18.18 26.66 41.66 ...19,238,246-270
🟡  src/components 55.3 44.6 56.64 56.06
🔴   ...vityTable.vue 18.75 0 28.57 19.35 115-185
🟡   BaseLayout.vue 69.56 53.84 60 69.56 172,219-231
🔴   CopyButton.vue 30 33.33 40 30 5,53-62
🟢   ...tActivity.vue 80.76 62.85 83.33 80.76 ...31-136,179,201
🟢   GrantNotes.vue 90 78.26 85 89.79 ...87,207,216-217
🔴   GrantsTable.vue 45.03 38.77 45 46.85 ...51-455,463-543
🔴   ...dUploader.vue 24.24 33.33 37.5 24.24 16,25,74-111
🔴   SearchFilter.vue 40.74 28 37.5 40 ...52,64,67,72-82
🔴   ShareGrant.vue 38.7 64.28 23.07 40 ...08-112,131-152
🟢   ...ivityItem.vue 96.15 90.9 100 96.15 64
🟡   UserAvatar.vue 70 75 100 62.5 35-37
🟢   ...eaderText.vue 100 100 100 100
🔴  ...ponents/Modals 29.21 44.34 33.1 32.94
🔴   ...anization.vue 17.39 65.62 30 22.22 1-14,149-178
🔴   AddTeam.vue 45.45 55.55 61.53 57.69 ...04,210,222-245
🔴   AddUser.vue 36.66 66.66 60 37.5 ...40,145,148-176
🔴   ...anization.vue 15.38 64.28 14.28 16.66 1-15,58-78
🔴   EditTeam.vue 18.18 26.43 33.33 20.83 ...29,208,216-301
🔴   EditUser.vue 21.05 66.66 25 22.22 1,101-128
🔴   ...ilsLegacy.vue 22.03 0 0 24.07 131,177,205-369
🟢   ...Followers.vue 83.33 88.88 57.14 83.33 10,114-115
🟡   ImportTeams.vue 50 41.17 50 53.33 28,64-69,81-82
🔴   ImportUsers.vue 42.85 60 40 46.15 29,65-80
🔴   ...archPanel.vue 27.9 15.78 23.52 28.57 ...68-178,211-255
🔴   SearchPanel.vue 21.62 63.26 32 27.58 ...77-380,386-458
🔴  src/helpers 18.12 17.92 18.6 18.88
🟢   constants.js 100 100 100 100
🟢   currency.js 100 100 100 100
🟡   dates.js 66.66 100 33.33 100
🔴   fetchApi.js 6 13.79 5.26 6.12 10-12,20-132
🔴   filters.js 4 0 0 4.54 19-51
🔴   form-helpers.js 0 0 0 0 5-82
🟡   gtag.js 77.77 90 75 77.77 12,51
🟢   testHelpers.js 100 100 100 100
🔴   ...patWarning.js 0 0 0 0 39-61
🟢  ...s/featureFlags 84.61 100 71.42 84.61
🟡   index.js 60 100 60 60 8,16
🟢   utils.js 100 100 100 100
🔴  src/mixin 20 0 28.57 20
🔴   ...zableTable.js 20 0 28.57 20 16-31,36-37,42
🔴  src/router 18.91 14.28 11.11 18.91
🔴   index.js 18.91 14.28 11.11 18.91 ...76-177,181-200
🟢  src/store 100 100 100 100
🟢   index.js 100 100 100 100
🔴  src/store/modules 2.82 0 4.48 2.94
🔴   agencies.js 5.26 100 8.33 5.55 13-70
🔴   alerts.js 20 100 20 20 10-24
🔴   grants.js 0.99 0 0.97 1.03 61-456
🔴   organization.js 33.33 100 33.33 33.33 21-25
🔴   roles.js 20 100 20 25 13-22
🔴   tenants.js 11.11 100 14.28 12.5 13-32
🔴   users.js 2.43 0 4.76 2.5 17-100
🔴  src/views 44.73 37.22 33.77 46.55
🔴   ...orterView.vue 25.58 51.85 18.18 26.82 ...,62,84,109-151
🟡   ...boardView.vue 50 17.64 50 52 89-98,114-125
🔴   ...tailsView.vue 32.97 14.28 17.07 33.69 ...96-435,441-462
🟢   GrantsView.vue 100 100 100 100
🟡   LoginView.vue 56 38.88 44.44 58.33 23,137-159
🟡   MyGrantsView.vue 77.77 66.66 66.66 77.77 1,69
🟡   ...ofileView.vue 78.26 80 42.85 78.26 1,32,63,136-140
🟢   NotFoundView.vue 100 100 100 100
🔴   ...tionsView.vue 47.05 57.14 41.66 53.33 ...97-100,114-118
🔴   ...ivityView.vue 46.42 23.8 43.75 46.42 ...01,114,120-134
🔴   TeamsView.vue 44.44 88.88 41.66 53.33 1,58,142,156-163
🟡   UsersView.vue 50 66.66 36.36 53.84 ...16-121,133-139
Coverage report for `packages/server`
St File % Stmts % Branch % Funcs % Lines Uncovered Line #s
🟡 All files 60.06 53.68 56.06 60.15
🟢  src 81.81 0 50 81.81
🟢   configure.js 81.81 0 50 81.81 47-54,83-85
🟢  src/arpa_reporter 100 71.42 100 100
🟢   configure.js 100 33.33 100 100 34-41
🟢   environment.js 100 100 100 100
🟢   use-request.js 100 100 100 100
🟡  src/arpa_reporter/db 50.73 45.88 50 51.9
🟡   arpa-subrecipients.js 53.19 50 38.46 54.54 23-60,101,113-122
🔴   reporting-periods.js 37.2 46.87 40 38.09 46,77-156
🟢   settings.js 100 83.33 100 100 13
🟡   uploads.js 50 28.57 52.38 51.42 18-29,84,99-124,141-150
🔴  src/arpa_reporter/lib 29.57 33.08 34.56 28.46
🟢   arpa-ec-codes.js 100 100 100 100
🔴   audit-report.js 21.44 19.35 24.19 21.32 ...28-529,554-684,732-758
🟡   ensure-async-context.js 75 100 50 100
🟢   format.js 90.62 90 90 91.3 41-42
🟡   log.js 75 50 50 75 13,25
🟡   preconditions.js 66.66 33.33 100 66.66 3
🔴   spreadsheet.js 9.09 0 0 9.09 15-32
🟢   validation-error.js 85.71 100 50 85.71 16
🔴  src/arpa_reporter/routes 40 14.92 14.28 40.6
🔴   agencies.js 22.58 0 0 23.33 13-21,26-53
🟡   application_settings.js 75 100 0 75 10-11
🟡   audit-report.js 68.91 58.33 100 68.91 57-58,64-78,100-116
🟢   exports.js 81.42 83.33 100 81.42 61-75,98-99
🔴   reporting-periods.js 20 0 0 20.43 ...25-137,143-149,154-180
🔴   subrecipients.js 23.8 0 0 23.8 12-13,17-27,31-48,52-63
🔴   uploads.js 28.28 7.89 9.09 29.16 ...33-154,164-166,173-180
🔴   users.js 19.6 0 0 20 15-35,39-44,48-81
🔴  src/arpa_reporter/services 44.55 34.79 47.55 44.83
🔴   generate-arpa-report.js 36.86 2.77 50 37.24 ...-975,984-997,1071-1138
🔴   get-template.js 21.62 0 0 21.62 18-79
🟡   persist-upload.js 69.66 90 73.07 69.76 ...64-206,227-241,279-301
🔴   records.js 20.75 0 11.11 21.15 38-204,221-276
🔴   revalidate-uploads.js 37.5 100 0 37.5 5-14
🔴   validate-upload.js 43.56 58.51 37.03 44.2 ...52,371,393,411-688,703
🟢   validation-rules.js 98.18 90 90.9 100 157,173
🟡  src/db 74.56 72.25 68.96 74.61
🟢   connection.js 100 50 100 100 6
🟢   constants.js 100 100 100 100
🟡   helpers.js 75 83.33 50 75 5,21-22
🟢   index.js 82.67 79.29 82.48 82.63 ...75-1441,1623-1624,1631
🟢   saved_search_migration.js 92 88.23 71.42 93.61 5,69,134
🔴   tenant_creation.js 10.58 2.7 0 11.11 15-40,[...[Comment body truncated]

@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2024 •
edited
Loading

Terraform Summary

Step Result
🖌 Terraform Format & Style ✅
⚙️ Terraform Initialization ✅
🤖 Terraform Validation ✅
📖 Terraform Plan ✅

Hint: If "Terraform Format & Style" failed, run terraform fmt -recursive from the terraform/ directory and commit the results.

Output

Validation Output 
stdout:
Success! The configuration is valid.


-------------------------------------
stderr:
Plan Summary
CHANGE RESOURCE
add module.website.aws_s3_object.origin_dist_artifact["assets/ActivityTable-DTd2sFX0.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/ActivityTable-DTd2sFX0.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/ArpaAnnualPerformanceReporterView-CDdDTksC.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/ArpaAnnualPerformanceReporterView-CDdDTksC.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/DashboardView-TapDTibd.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/DashboardView-TapDTibd.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsLegacy-Cz_aXguf.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsLegacy-Cz_aXguf.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsView-D5GpA3Du.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsView-D5GpA3Du.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsTable-DXygXY3I.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsTable-DXygXY3I.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsView-D8QTMq0E.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsView-D8QTMq0E.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyGrantsView-mTLphgQW.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyGrantsView-mTLphgQW.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyProfileView-kfN1a99H.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyProfileView-kfN1a99H.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/NotFoundView-DG0C-4Kw.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/NotFoundView-DG0C-4Kw.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/OrganizationsView-CKQRlZ5P.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/OrganizationsView-CKQRlZ5P.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecentActivityView-CPrFA33M.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecentActivityView-CPrFA33M.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecordUploader-C9DgaHrg.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecordUploader-C9DgaHrg.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/TeamsView-BSGssl7p.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/TeamsView-BSGssl7p.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/UsersView-shQyCqcR.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/UsersView-shQyCqcR.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/arpa_reporter-BDVWdOGe.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/arpa_reporter-BDVWdOGe.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/form-helpers-BmCDRMDS.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/form-helpers-BmCDRMDS.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/main-yGBsrI0n.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/main-yGBsrI0n.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/style-Cjl3lQxh.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/style-Cjl3lQxh.js.map"]
update module.api.aws_ecs_service.default[0]
module.api.module.grant_digest_scheduled_task.aws_iam_role_policy.default[0]
module.api.module.grant_digest_scheduled_task.aws_scheduler_schedule.default[0]
module.arpa_audit_report.aws_ecs_service.default
module.arpa_treasury_report.aws_ecs_service.default
module.consume_grants.aws_ecs_service.default
module.website.aws_s3_object.deploy-config[0]
module.website.aws_s3_object.origin_dist_artifact["arpa_reporter/index.html"]
module.website.aws_s3_object.origin_dist_artifact["index.html"]
recreate module.api.aws_ecs_task_definition.default[0]
module.arpa_audit_report.aws_ecs_task_definition.consumer
module.arpa_treasury_report.aws_ecs_task_definition.consumer
module.consume_grants.aws_ecs_task_definition.consume_grants
delete module.website.aws_s3_object.origin_dist_artifact["assets/ActivityTable-YqgOOPXw.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/ActivityTable-YqgOOPXw.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/ArpaAnnualPerformanceReporterView-DKVm08Xg.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/ArpaAnnualPerformanceReporterView-DKVm08Xg.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/DashboardView-VsspTFBS.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/DashboardView-VsspTFBS.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsLegacy-CVlel6Ry.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsLegacy-CVlel6Ry.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsView-DJGnKTa1.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantDetailsView-DJGnKTa1.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsTable-jhzN9SPv.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsTable-jhzN9SPv.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsView-BrsJtfrf.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/GrantsView-BrsJtfrf.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyGrantsView-BjfVdyV4.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyGrantsView-BjfVdyV4.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyProfileView-B3fT5aIb.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/MyProfileView-B3fT5aIb.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/NotFoundView-BNQmXSgE.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/NotFoundView-BNQmXSgE.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/OrganizationsView-D-fJvjQ_.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/OrganizationsView-D-fJvjQ_.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecentActivityView-C0M1DIV9.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecentActivityView-C0M1DIV9.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecordUploader-BabAXXQR.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/RecordUploader-BabAXXQR.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/TeamsView-LLkFMdO8.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/TeamsView-LLkFMdO8.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/UsersView-CQQQ-iaa.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/UsersView-CQQQ-iaa.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/arpa_reporter-B11UrlnO.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/arpa_reporter-B11UrlnO.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/form-helpers-k5gBX_5e.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/form-helpers-k5gBX_5e.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/main-DxZvaAMs.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/main-DxZvaAMs.js.map"]
module.website.aws_s3_object.origin_dist_artifact["assets/style-Lb7S2aAW.js"]
module.website.aws_s3_object.origin_dist_artifact["assets/style-Lb7S2aAW.js.map"]

Pusher: @lsr-explore, Action: pull_request_target, Workflow: Continuous Integration

@masimons masimons changed the title Ms hide forecasted grants Hide forecasted grants while UI is being developed to support forecasted grants Sep 3, 2024
TylerHendrickson
TylerHendrickson reviewed Sep 5, 2024
View reviewed changes
Copy link
Member

@TylerHendrickson TylerHendrickson left a comment

Choose a reason for hiding this comment

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

@masimons Confirming that the direction this is headed makes sense to me 👍

I'll mention that I think it could be useful to have an environment variable control the exclusion of forecasted grants. Due to how we ship and run Docker images, it's not something that we could toggle in a Staging/Prod environment without re-deploying, but 1) it might be helpful for development purposes, and 2) might make it easier to activate once we get to the point where we want to start including forecasted grants in one or both of those environments.

@masimons
Copy link
Contributor Author

@TylerHendrickson thanks for checking this out, I missed this comment. Out of curiosity, do you have an example off the top of your head where we use an env var in that way? I was looking into how we use feature flags, but it seems that those are scoped to the client side?

@github-actions github-actions bot added Infra Issues related to the infrastructure underlying all the tools. terraform Pull requests that update Terraform code labels Sep 17, 2024
@TylerHendrickson
Copy link
Member

@masimons re

Out of curiosity, do you have an example off the top of your head where we use an env var in that way? I was looking into how we use feature flags, but it seems that those are scoped to the client side?

The implementation details for #3060 provide some instructions for implementing an API-side feature flag using a SHARE_TERMINOLOGY_ENABLED environment variable (plus a client-side feature flag, which doesn't apply here). This was implemented by PR #3147.

Fundamentally, this sort of thing would entail the following:

  • An env var default value for dev environments, defined in packages/server/.env.example.
  • Configuring the value for Staging environments as a key/value pair in the api_container_environment input variable in terraform/staging.tfvars.
  • Configuring the value for Production environments via the same input variable in terraform/prod.tfvars.

I'd recommend that an environment variable that provides a binary feature flag be evaluated as a string using strict equality, e.g. my_feature_is_enabled = process.env.MY_FEATURE_ENABLED === 'true'.

@masimons masimons marked this pull request as ready for review September 30, 2024 03:50
TylerHendrickson
TylerHendrickson requested changes Oct 11, 2024
View reviewed changes
Copy link
Member

@TylerHendrickson TylerHendrickson left a comment

Choose a reason for hiding this comment

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

@masimons I really like how this implementation parameterizes the flag in src/db/index.js and allows the caller to decide whether to pull in the toggle from env var or something else. This is looking good overall!

I did notice in local testing that setting SHOW_FORECASTED_GRANTS=false in packages/server/.env doesn't seem to have the desired effect (I could still see forecasted grants in search results), although when the env var is altogether missing, forecasted grants are hidden like we'd expect.

I believe this is because the packages/server/db/index.js functions are expecting a true boolean value (or else undefined, so that the arg default of false in the function signature is used), but when a value is provided from process.env.SHOW_FORECASTED_GRANTS, it's a string rather than a boolean, and !"false" is evaluating to true. I think the simplest way to resolve this is to have callers explicitly check whether process.env.SHOW_FORECASTED_GRANTS === "true", which I provided in a comment suggestion.

packages/server/src/routes/grants.js Outdated Show resolved Hide resolved
@lsr-explore lsr-explore force-pushed the ms-hide-forecasted-grants branch 2 times, most recently from 029057a to 321c973 Compare October 16, 2024 20:23
@lsr-explore
Copy link
Contributor

@masimons I really like how this implementation parameterizes the flag in src/db/index.js and allows the caller to decide whether to pull in the toggle from env var or something else. This is looking good overall!

I did notice in local testing that setting SHOW_FORECASTED_GRANTS=false in packages/server/.env doesn't seem to have the desired effect (I could still see forecasted grants in search results), although when the env var is altogether missing, forecasted grants are hidden like we'd expect.

I believe this is because the packages/server/db/index.js functions are expecting a true boolean value (or else undefined, so that the arg default of false in the function signature is used), but when a value is provided from process.env.SHOW_FORECASTED_GRANTS, it's a string rather than a boolean, and !"false" is evaluating to true. I think the simplest way to resolve this is to have callers explicitly check whether process.env.SHOW_FORECASTED_GRANTS === "true", which I provided in a comment suggestion.

@TylerHendrickson , @masimons - I've accepted Ty's changes - this should be ready for review

@lsr-explore lsr-explore mentioned this pull request Oct 21, 2024
Open
7 tasks
TylerHendrickson
TylerHendrickson requested changes Oct 21, 2024
View reviewed changes
Copy link
Member

@TylerHendrickson TylerHendrickson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@lsr-explore Thanks for the updates. I think we need the strict === 'true' comparison for all usages of the environment variable, so I added a few additional comment suggestions to address those as well. I think that I covered all of them, but feel free to double check me on that 🙂

packages/server/src/lib/email.js Outdated Show resolved Hide resolved
packages/server/src/routes/grants.js Outdated Show resolved Hide resolved
packages/server/src/routes/grants.js Outdated Show resolved Hide resolved
packages/server/src/routes/grants.js Outdated Show resolved Hide resolved
@lsr-explore lsr-explore mentioned this pull request Oct 21, 2024
Open
7 tasks
@lsr-explore
Copy link
Contributor

@lsr-explore Thanks for the updates. I think we need the strict === 'true' comparison for all usages of the environment variable, so I added a few additional comment suggestions to address those as well. I think that I covered all of them, but feel free to double check me on that 🙂

Sorry that I missed this @TylerHendrickson . I applied the changes and did another sanity check of the code. I noticed a couple additional places where the flag needed to be sent and added a commit.

I noticed that the db.getGrantsInterested didn't have this check. This would only be an issue if we needed to turn off grants after enabling it and user had clicked follow.

Do you think we need the check on getGrantsInterested?

@masimons
Copy link
Contributor Author

I noticed that the db.getGrantsInterested didn't have this check. This would only be an issue if we needed to turn off grants after enabling it and user had clicked follow.

Do you think we need the check on getGrantsInterested?

That's a fair point - I don't know if that particular case came up when we were thinking about this

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

@TylerHendrickson TylerHendrickson Awaiting requested review from TylerHendrickson

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Infra Issues related to the infrastructure underlying all the tools. javascript Pull requests that update Javascript code terraform Pull requests that update Terraform code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@masimons @TylerHendrickson @lsr-explore