-
Notifications
You must be signed in to change notification settings - Fork 7
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
Issue #54 - User story 3.6 - Scheduling audits #233
Conversation
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
==========================================
- Coverage 93.03% 90.75% -2.29%
==========================================
Files 36 38 +2
Lines 1077 1168 +91
==========================================
+ Hits 1002 1060 +58
- Misses 75 108 +33
Continue to review full report at Codecov.
|
NG Lint Report |
NG Lint Report |
Pylint Report '+ ######ALL JOBS####### |
NG Lint Report |
Pylint Report '+ ######ALL JOBS####### |
NG Lint Report |
Pylint Report '+ ######ALL JOBS####### |
NG Lint Report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good in general, please have a look at my comments
::ng-deep .mat-checkbox-checked.mat-accent .mat-checkbox-ripple .mat-ripple-element { | ||
opacity: 0.03 !important; | ||
background-color: map-get($alta-primary, A100); | ||
} | ||
|
||
::ng-deep .mat-checkbox-checked.mat-accent .mat-checkbox-background, .mat-checkbox-indeterminate.mat-accent .mat-checkbox-background { | ||
background-color: map-get($alta-primary, A100); | ||
} | ||
|
||
.button-padding { | ||
margin: 5px; | ||
} | ||
|
||
::ng-deep .mat-checkbox-label { | ||
padding-top: 5px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not use ng-deep here.
Last time we tried to use this, it ended up modifying the scss for the entire material library and it ended up messing up other components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also tend to avoid it but unfortunately for checkbox specifically, I have no choice but to use ng-deep in order to force the style down to the child as it won't overwrite the CSS.
updateAllCheckbox(type: string): void { | ||
if (type === 'dayCheckbox') { | ||
this.allDaysChecked = | ||
this.recurrenceDay.subCheckBox != null && | ||
this.recurrenceDay.subCheckBox.every((t) => t.checked); | ||
this.errorMessageCheckboxDay = ' '; | ||
} else if (type === 'monthCheckbox') { | ||
this.allMonthsChecked = | ||
this.recurrenceMonth.subCheckBox != null && | ||
this.recurrenceMonth.subCheckBox.every((t) => t.checked); | ||
this.errorMessageCheckboxMonth = ' '; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't check to see if this would work, with the HTML, but I was wondering, could you split this into 2 functions and avoid having to pass in a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateAllCheckbox has now been split into two function named: updateCheckboxDay and updateCheckboxMonth.
someCheckbox(type: string): boolean { | ||
if (type === 'dayCheckbox') { | ||
if (this.recurrenceDay.subCheckBox == null) { | ||
return false; | ||
} | ||
return ( | ||
this.recurrenceDay.subCheckBox.filter((t) => t.checked).length > 0 && | ||
!this.allDaysChecked | ||
); | ||
} else if (type === 'monthCheckbox') { | ||
if (this.recurrenceMonth.subCheckBox == null) { | ||
return false; | ||
} | ||
return ( | ||
this.recurrenceMonth.subCheckBox.filter((t) => t.checked).length > 0 && | ||
!this.allMonthsChecked | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same applies here, I'm not sure if this would fit with the HTML, but if yes, moving this into 2 functions would make more sense to avoid having to check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
someCheckbox has now been split into two function named: someCheckboxDay and someCheckboxMonth.
setAllCheckbox(checked: boolean, type: string): void { | ||
if (type === 'dayCheckbox') { | ||
this.allDaysChecked = checked; | ||
if (this.recurrenceDay.subCheckBox == null) { | ||
return; | ||
} | ||
this.recurrenceDay.subCheckBox.forEach((t) => (t.checked = checked)); | ||
} else if (type === 'monthCheckbox') { | ||
this.allMonthsChecked = checked; | ||
if (this.recurrenceMonth.subCheckBox == null) { | ||
return; | ||
} | ||
this.recurrenceMonth.subCheckBox.forEach((t) => (t.checked = checked)); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concept as previous 2 comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setAllCheckbox has now been split into two function named: setAllCheckboxDay and setAllCheckboxMonth.
const year = this.startDate.getFullYear(); | ||
const month = ((this.startDate.getMonth() + 1).toString().length === 1) ? | ||
('0' + (this.startDate.getMonth() + 1)) : ((this.startDate.getMonth() + 1)); | ||
const day = (this.startDate.getDate().toString().length === 1) ? ('0' + (this.startDate.getDate())) : (this.startDate.getDate()); | ||
const hour = (parseInt(this.startTime.split(':')[0], 10).toString().length === 1) ? | ||
('0' + (parseInt(this.startTime.split(':')[0], 10))) : (parseInt(this.startTime.split(':')[0], 10)); | ||
const minute = (parseInt(this.startTime.split(':')[1], 10).toString().length === 1) ? | ||
('0' + (parseInt(this.startTime.split(':')[1], 10))) : (parseInt(this.startTime.split(':')[1], 10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives me a headache to look at and try to understand. Can we take each of these statements and inline them with a function instead? This would make them much more readable if the function names are properly defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you did, but I would prefer each of these assignments were done with their own function instead of dumping the functionality into an already long function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I see what you meant! Sorry I misunderstood earlier. This has been fixed now 😄
}); | ||
}); | ||
} | ||
|
||
formTemplate(temp: { [s: string]: unknown; } | ArrayLike<unknown>): any { | ||
const createdTemplate: any = {}; | ||
Object.entries(temp).forEach(([key, value]) => { | ||
console.log(key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been removed.
Pylint Report '+ ######ALL JOBS####### |
NG Lint Report |
server/audit/utils.py
Outdated
for query in queries: | ||
for category, value in zip(sorted_keys, query): | ||
kwargs[category] = value | ||
|
||
# Fetching the inventory items corresponding to the criteria selected | ||
# for all combinations | ||
inventory_items = Item.objects.filter(**kwargs) | ||
|
||
# Appending the element of the Queryset object to the list | ||
# (Didn't find a way to cast it to a list) | ||
for i in range(len(inventory_items)): | ||
inventory_items_to_audit.append(inventory_items[i]._id) | ||
|
||
return inventory_items_to_audit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a TODO here to look further into a more efficient solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
Pylint Report '+ ######ALL JOBS####### |
NG Lint Report |
Pylint Report '+ ######ALL JOBS####### |
NG Lint Report |
Pylint Report '+ ######ALL JOBS####### |
NG Lint Report |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good now, thank you for the changes!
Issue #54
This PR adds the functionality to schedule an audit template. When an inventory managers create an audit template, at the bottom they will see a new section named Scheduling. In this section they have the option to trigger an audit at a scheduled time specified. There are 2 ways to trigger: One time trigger, or at a recurrence time by expanding the recurrence tab and specifying the available option.
On the server end, you can see in the console output when the scheduled audit starts.