clients
clients copied to clipboard
[PM-5382] Add Sync Purpose
Type of change
- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other
Objective
This will help us track which sync types are happening so we can better track when they are being to eager with their syncing.
What to review
- That the purpose I chose for your code makes sense
- That I spelled everything correctly
Code changes
- ApiService - Take the new purpose and set it into a header
- SyncService - make a purpose now required.
- Everything else adding a purpose to the best of my ability.
Screenshots
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
Can we enumerate these so they are centralized and then enforce some naming standards and clarity?
Don't we include device type in headers? Why do we need the client in the purpose string?
@withinfocus @eliykat I did think about doing something like this but was worried it would actually encourage reuse of the purpose string. Since an enum or string union is generally used as a list of allowed values but in our case it would be a list of not allowed values until you add your single entry and then that is the only value you're allowed to use.
Plus an enum would only indicate we have a duplicate if we named the identifier the same but not if we named the identifier different but still had the value the same. The union wouldn't save us at all, it will just collapse the repeated entry into one. (Ref)
@jlf0dev Very good point, for some reason I thought that was only being added to some api calls not all of them. Think it's alright to repeat the same purpose string in those cases then?
Think it's alright to repeat the same purpose string in those cases then?
Seems like it would be be ok. I do like your goal here of keeping them unique enough to identify where it came from, but we might be able to find the client
Checkmarx One – Scan Summary & Details – 5305327a-e85d-4ea8-98bd-94beb65ca502
New Issues
Severity | Issue | Source File / Package | Checkmarx Insight |
---|---|---|---|
![]() |
Use_of_Broken_or_Risky_Cryptographic_Algorithm | /apps/browser/src/background/service-factories/vault-timeout-service.factory.ts: 97 | Attack Vector |
Codecov Report
Attention: Patch coverage is 5.26316%
with 72 lines
in your changes are missing coverage. Please review.
Project coverage is 27.74%. Comparing base (
3caa6cb
) to head (bc7f072
). Report is 35 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #7289 +/- ##
==========================================
+ Coverage 27.70% 27.74% +0.03%
==========================================
Files 2392 2411 +19
Lines 69356 69856 +500
Branches 12934 12992 +58
==========================================
+ Hits 19218 19381 +163
- Misses 48658 48972 +314
- Partials 1480 1503 +23
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.