Skip to content

work period endpoint added #177

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

Merged
merged 1 commit into from
Apr 4, 2021
Merged

work period endpoint added #177

merged 1 commit into from
Apr 4, 2021

Conversation

eisbilir
Copy link
Member

@eisbilir eisbilir commented Apr 3, 2021

No description provided.

@maxceem maxceem changed the base branch from dev to feature/work-periods April 4, 2021 10:27
Comment on lines +244 to +248
// change the date format to match with database model
if (data.startDate && data.endDate) {
data.startDate = moment(data.startDate).format('YYYY-MM-DD')
data.endDate = moment(data.endDate).format('YYYY-MM-DD')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@eisbilir we have the logic change the date format to match with database model during the update, but not during creating. I wonder what is the correct way: can we remove this logic during update if it's not needed, or should we add the same logic during creating if its needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

We define startDate and endDate formats in Joi.date().format('YYYY-MM-DD')
This works as expected, Joi validates the date formats as we want, without timestamps.
But the problem is in here: src/common/logger.js#149 --> const normalized = Joi.attempt(value, method.schema)
After joi validates our dates which is in 'YYYY-MM-DD' format with above command; it returns dates localized with timestamps. So normalized variable becomes 2021-04-04T12:04:09.492Z. I believe this is a bug.

This is why I converted it to desired format back.
Now, I tried a different logic and it seems that we don't need that conversion anymore because sequelize will convert them back to 'YYYY-MM-DD'. But we also have remove the line #263 const result = _.assign(workPeriod.dataValues, data) and return updated.dataValues directly. With this way we won't need that conversion anymore.

Comment on lines +235 to +243
if (data.startDate && !data.endDate) {
const date = new Date(data.startDate)
date.setDate(date.getDate() + 6)
data.endDate = date
} else if (!data.startDate && data.endDate) {
const date = new Date(data.endDate)
date.setDate(date.getDate() - 6)
data.startDate = date
}
Copy link
Contributor

@maxceem maxceem Apr 4, 2021

Choose a reason for hiding this comment

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

@eisbilir let's put this logic If one of the dates are missing then auto-calculate it into a reusable method and use it in both create and updated methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay

@maxceem maxceem merged commit 5e02e35 into topcoder-platform:feature/work-periods Apr 4, 2021
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.

None yet

2 participants