-
Notifications
You must be signed in to change notification settings - Fork 1
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
Ben/lmb 1288 end date mandatarissen #465
Conversation
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.
Just looked at the code for now
@@ -123,7 +125,7 @@ export default class MandatarissenPersoonMandatenController extends Controller { | |||
newMandataris.id | |||
); | |||
|
|||
mandataris.einde = dateNow; | |||
mandataris.einde = moment(dateNow).add(1, 'days').startOf('day').toDate(); |
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.
Creating the mandataris end date, could this be an object or a util method?
You've set the date in two ways on three places
# first
moment(date).add(1, 'days').toDate();
# second
moment(date).add(1, 'days').startOf('day').toDate();
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'll add a helper
app/models/mandataris.js
Outdated
get displayEinde() { | ||
if (!this.einde) { | ||
return this.einde; | ||
} | ||
return moment(this.einde).subtract(1, 'days').toDate(); | ||
} |
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.
- update mandataris
date + 1day
- show mandataris einde
date + 1day + 1day
?
Or is this a mind thing?
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 says subtract
so it is date + 1day - 1day
@@ -55,6 +55,9 @@ export default class DateInputComponent extends Component { | |||
} | |||
|
|||
processDate(date) { | |||
if (this.args?.endOfDay) { |
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.
Would name the argument isMandatarisEndDate
or something, endOfDay could be anything so to speak
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.
The problem is this is not exclusive to mandatarissen, so I do keep it a bit abstract so it is useful in other cases as well. e.g. for bestuursorganen in de tijd.
@@ -3,7 +3,7 @@ | |||
{{moment-format @mandataris.start "DD-MM-YYYY"}} | |||
</td> | |||
<td> | |||
{{moment-format @mandataris.einde "DD-MM-YYYY"}} | |||
{{moment-format @mandataris.displayEinde "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.
Maybe we can add a getter in the mandataris model for a formattedStartDate and end date?
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.
context is always a bit different, so quite difficult to implement
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.
no lets keep this in the templates as is, it's nice to know what format you'll get there
@@ -104,7 +105,7 @@ export default class MandatarissenUpdateState extends Component { | |||
if (!this.args.mandataris.einde) { | |||
return this.args.mandataris.status; | |||
} | |||
if (this.args.mandataris.einde.getTime() < new Date().getTime()) { | |||
if (this.args.mandataris.einde.getTime() <= new Date().getTime()) { |
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 don't think you can just change the sign here. getTime returns the time since epoch in milliseconds. i don't think the equals matters right? And the logic is still good because the mandataris is still in function until the end of the day?
Description
Show mandataris end dates, as the day before, since we use the start of the day as end now. Also allow forms to handle this, they show a certain date for end dates, but save the start of the next day.
How to test
Add a mandataris, check if it's end date is saved correctly, also try updating state, changing the mandataris, and see if the end date is displayed correctly in different views.
Links to other PR's