clients icon indicating copy to clipboard operation
clients copied to clipboard

[AC-2302] Extract device approve/deny logic into a service

Open r-tome opened this issue 1 year ago • 3 comments

Type of change

- [ ] Bug fix
- [ ] New feature development
- [X] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Move all device approve / deny code from the component into a service.

Code changes

  • bitwarden_license/bit-common/*: Jest configs for bit-common
  • bitwarden_license/bit-common/src/admin-console/services/auth-requests/*: Moved files from bit-web to bit-common
  • bitwarden_license/bit-common/src/admin-console/services/auth-requests/organization-auth-request-api.service.ts: Renamed and removed @Injectable() decoration
  • bitwarden_license/bit-common/src/admin-console/services/auth-requests/organization-auth-request.service.spec.ts: Unit tests for OrganizationAuthRequestService
  • bitwarden_license/bit-common/src/admin-console/services/auth-requests/organization-auth-request.service.ts: Created service with logic extracted from component
  • bitwarden_license/bit-web/src/app/admin-console/organizations/core/*: Removed files as they are not necessary any longer
  • bitwarden_license/bit-web/src/app/admin-console/organizations/manage/device-approvals/device-approvals.component.ts: Extracted logic into OrganizationAuthRequestService
  • bitwarden_license/bit-web/src/app/admin-console/organizations/organizations.module.ts: Configured DI manually for OrganizationAuthRequestApiService and OrganizationAuthRequestService

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

r-tome avatar Apr 18 '24 19:04 r-tome

Codecov Report

Attention: Patch coverage is 55.55556% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 27.91%. Comparing base (4131aa9) to head (77675ad).

Files Patch % Lines
...age/device-approvals/device-approvals.component.ts 0.00% 10 Missing :warning:
...-requests/organization-auth-request-api.service.ts 0.00% 2 Missing :warning:
...auth-requests/organization-auth-request.service.ts 90.90% 1 Missing and 1 partial :warning:
...sole/organizations/organizations-routing.module.ts 0.00% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8818      +/-   ##
==========================================
+ Coverage   27.89%   27.91%   +0.01%     
==========================================
  Files        2357     2356       -1     
  Lines       69616    69619       +3     
  Branches    13119    13117       -2     
==========================================
+ Hits        19421    19435      +14     
+ Misses      48656    48644      -12     
- Partials     1539     1540       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 18 '24 19:04 codecov[bot]

Logo Checkmarx One – Scan Summary & Details822a5862-0c58-4d81-a32a-4baf6860795f

No New Or Fixed Issues Found

github-actions[bot] avatar Apr 18 '24 20:04 github-actions[bot]

I had to spend a bit more time on this, here's the summary.

In https://github.com/bitwarden/clients/pull/8818/commits/5e66416c13dff18d61d09a2431cec4609140b1c1 I removed the @bitwarden/bit-common path mapping from apps/web, because the web vault is OSS code. It should never import BL code, otherwise it's no longer purely OSS. Instead, BL code is our entrypoint, and it imports OSS code. (You can see this because npm run build:bit points to a webpack file in the bit-web directory.) ((When you start updating the CLI, it will need a similar configuration.))

However, npm run build:bit was still using the tsconfig.json in the apps/web directory, because that's where the command was being run from. So removing that mapping broke the bit build.

The Angular Webpack Plugin does provide an option to specify the location of tsconfig, but it looks like at some stage it changed name: the docs call it tsconfig whereas our configuration uses tsConfigPath, so that wasn't actually working as intended.

Once I fixed that to use the bit-web tsconfig, the build broke because that tsconfig isn't fully set up. I had to copy across the configuration from apps/web/tsconfig.json and update relative paths, and then for good measure, remove the BL entrypoint from apps/web/tsconfig.json. (One annoying thing here is that the tsconfig extends option isn't very robust; as soon as you set a top-level configuration option in the child tsconfig, it'll blast over the parent tsconfig, so lots of copy/paste is required here to preserve nested config objects.)

On the plus side, this is now working how it should 😁 However you and @addisonbeck should review my commits.

eliykat avatar May 03 '24 03:05 eliykat