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

[backend] add missing backend feature to exclusion list (#8941) #9177

Open
wants to merge 4 commits into
base: release/6.5.0
Choose a base branch
from

Conversation

JeremyCloarec
Copy link
Contributor

@JeremyCloarec JeremyCloarec commented Nov 28, 2024

Proposed changes

  • add exclusion list check in indicator edit
  • add handling of file update in exclusion list field patch
  • restrict exclusion list file size. Limits file size to 10MB by default
  • add cache status query

Related issues

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case (coverage and e2e)
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

Attention: Patch coverage is 55.81395% with 38 lines in your changes missing coverage. Please review.

Project coverage is 64.96%. Comparing base (fa196f2) to head (6840f87).

Files with missing lines Patch % Lines
.../src/modules/exclusionList/exclusionList-domain.ts 51.61% 30 Missing ⚠️
...-graphql/src/modules/indicator/indicator-domain.ts 60.00% 8 Missing ⚠️
Additional details and impacted files
@@                Coverage Diff                @@
##           release/6.5.0    #9177      +/-   ##
=================================================
+ Coverage          64.93%   64.96%   +0.02%     
=================================================
  Files                626      626              
  Lines              59722    59771      +49     
  Branches            6620     6635      +15     
=================================================
+ Hits               38783    38829      +46     
- Misses             20939    20942       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JeremyCloarec JeremyCloarec marked this pull request as draft November 29, 2024 08:12
@JeremyCloarec JeremyCloarec marked this pull request as ready for review December 2, 2024 08:40
type ExclusionListCacheStatus {
refreshAskDate: String!
cacheDate: String!
allNodeCacheUpdated: Boolean!
Copy link
Member

@SouadHadjiat SouadHadjiat Dec 2, 2024

Choose a reason for hiding this comment

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

Why String for the dates and not DateTime ? and also all these values can be null I guess ?
Also the naming is not ideal, here are my suggestions if you like them better:

cacheDate: DateTime
isCacheRebuildInProgress: Boolean!

So that the frontend can use this isCacheRebuildInProgress to display a progress status if it's true, and if it's false, then cache is up to date. Do we need more info ?

Copy link
Member

Choose a reason for hiding this comment

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

or maybe we would like the refreshAskDate for debug purposes ?

@SouadHadjiat
Copy link
Member

Tested locally:

  • creation & edition of indicator with an excluded pattern throws an error ✅
  • Creation of an exlusion list with the same name replaces the previous one : I think it can be an issue, better to not replace existing exclusion lists, and create a new one with the same name.

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.

2 participants