-
Notifications
You must be signed in to change notification settings - Fork 15
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
Release 3.3.4-alpha #1858
Release 3.3.4-alpha #1858
Conversation
Various updates
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.
Thanks. Mostly looks good to me. Just some minor comments below.
@@ -127,6 +128,10 @@ export class TasksService { | |||
return task.completed && this.isToday(task.timeCompleted) | |||
} | |||
|
|||
wasTaskValidToday(task) { | |||
return this.isToday(task.timestamp) |
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.
what is the reason for this 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.
will this still work if completion window is long, so the task timestamp is before today?
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 added the check to the getTasks method:
t => !this.isTaskExpired(t) || this.wasTaskCompletedToday(t) || this.wasTaskValidToday(t) |
It will check if the task is not expired (if the completion window is long), if the task was completed today, and if the task was valid today. This was to keep expired tasks if they were valid in the same day to keep track of task progress of the day. Otherwise, they are not included in the day's progress.
@@ -281,7 +287,7 @@ export class QuestionsPageComponent implements OnInit { | |||
const currentQs = this.getCurrentQuestions() | |||
if (!currentQs) return | |||
this.isRightButtonDisabled = | |||
!this.questionsService.isAnyAnswered(currentQs) && | |||
!this.questionsService.areAllAnswered(currentQs) && | |||
!this.questionsService.getIsAnyNextEnabled(currentQs) |
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.
Could there be cases even in matrix types where some questions are required but some are optional on the same page?
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.
Yes, but we currently don't support whether individual questions are required. They are configured per input type. But yes we can add support for this since it is a field in the Redcap questionnaire.
value = {} | ||
DEFAULT_DATE_FORMAT = 'DD/MM/YYYY' |
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 think this is enough for now, but would be good to do this via remote config (or protocol). Please create an issue so it can be handled later.
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.
Added here: #1859
ios/App/App/Info.plist
Outdated
@@ -83,5 +83,10 @@ | |||
</array> | |||
<key>UIViewControllerBasedStatusBarAppearance</key> | |||
<true/> | |||
<key>NSAppTransportSecurity</key> | |||
<dict> | |||
<key>NSAllowsArbitraryLoads</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.
what is this needed for (http urls?). I assume this will this be less secure?
If required i would like this to be configurable per project like we do in the pRMT app through remote config (by default it should be secure option)
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.
Ah yes I was just copying this from the dev
branch because I thought it was needed. Removed now.
DD/MM/YYYY
format