Issues with "number of days in a week fix"

Mansi_Singhal · 15 September 2014 09:57

Hey Lars ,

I have taken revision number 16720 from trunk in our branch. What i observed is Number of days in a week are still taken as 6. DateUtilsTest class also assumes number of days as 6.

We were assuming number of days in a week as 7.

Is it a bug or Do I need to take changes from some other revision too.

···

–
Regards,

Mansi Singhal

Lars · 15 September 2014 12:05

Hi Mansi,

that’s strange. Actually in rev 16720 the important change is that analytics uses Period.getDaysInPeriod(), which adds one day to the result (since period end date is inclusive). See line 331 in DefaultAnalyticsService. Are you sure its not a cache issue? Let me double check on my side.

Lars

···

On Mon, Sep 15, 2014 at 11:57 AM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars ,

I have taken revision number 16720 from trunk in our branch. What i observed is Number of days in a week are still taken as 6. DateUtilsTest class also assumes number of days as 6.

We were assuming number of days in a week as 7.

Is it a bug or Do I need to take changes from some other revision too.

Regards,

Mansi Singhal

–

Mansi_Singhal · 15 September 2014 12:41

Hey Lars,

I checked it again clearing cache. Issue still remains the same. Can you please check it once again on your side.

···

On Mon, Sep 15, 2014 at 5:35 PM, Lars Helge Øverland larshelge@gmail.com wrote:

Hi Mansi,

that’s strange. Actually in rev 16720 the important change is that analytics uses Period.getDaysInPeriod(), which adds one day to the result (since period end date is inclusive). See line 331 in DefaultAnalyticsService. Are you sure its not a cache issue? Let me double check on my side.

Lars

–
Regards,

Mansi Singhal

+91 9900246052

On Mon, Sep 15, 2014 at 11:57 AM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars ,

I have taken revision number 16720 from trunk in our branch. What i observed is Number of days in a week are still taken as 6. DateUtilsTest class also assumes number of days as 6.

We were assuming number of days in a week as 7.

Is it a bug or Do I need to take changes from some other revision too.

Regards,

Mansi Singhal

–

Lars · 15 September 2014 14:25

Hi again,

I have set up an example locally with a few indicators called “x number of cases per day” where numerator is no of cases and denominator is [days] for a week. See attached. It works correctly.

Could it be a build issue?
Can you please check in your branch on Period.java, method getDaysInPeriod, around line 247, and see if one is added to the number of days in period?

public int getDaysInPeriod()

{

Days days = Days.daysBetween( new DateTime( startDate ), new DateTime( endDate ) );

return days.getDays() + 1;

}

Lars

···

On Mon, Sep 15, 2014 at 2:41 PM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars,

I checked it again clearing cache. Issue still remains the same. Can you please check it once again on your side.

On Mon, Sep 15, 2014 at 5:35 PM, Lars Helge Øverland larshelge@gmail.com wrote:

Hi Mansi,

that’s strange. Actually in rev 16720 the important change is that analytics uses Period.getDaysInPeriod(), which adds one day to the result (since period end date is inclusive). See line 331 in DefaultAnalyticsService. Are you sure its not a cache issue? Let me double check on my side.

Lars

Regards,

Mansi Singhal

+91 9900246052

–

On Mon, Sep 15, 2014 at 11:57 AM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars ,

I have taken revision number 16720 from trunk in our branch. What i observed is Number of days in a week are still taken as 6. DateUtilsTest class also assumes number of days as 6.

We were assuming number of days in a week as 7.

Is it a bug or Do I need to take changes from some other revision too.

Regards,

Mansi Singhal

–

Mansi_Singhal · 16 September 2014 05:46

Hello,

Thanks Lars for your suggestions. Our branch doesn’t had that method. I have taken those changes and it just works fine.

Actually, commit no: 16720 doesn’t contain that change. Period class might have changed in some other previous commit.

···

On Mon, Sep 15, 2014 at 7:55 PM, Lars Helge Øverland larshelge@gmail.com wrote:

Hi again,

I have set up an example locally with a few indicators called “x number of cases per day” where numerator is no of cases and denominator is [days] for a week. See attached. It works correctly.

Could it be a build issue?

Can you please check in your branch on Period.java, method getDaysInPeriod, around line 247, and see if one is added to the number of days in period?

public int getDaysInPeriod()

{

Days days = Days.daysBetween( new DateTime( startDate ), new DateTime( endDate ) );

return days.getDays() + 1;

}

Lars

–
Regards,

Mansi Singhal

+91 9900246052

On Mon, Sep 15, 2014 at 2:41 PM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars,

I checked it again clearing cache. Issue still remains the same. Can you please check it once again on your side.

On Mon, Sep 15, 2014 at 5:35 PM, Lars Helge Øverland larshelge@gmail.com wrote:

Hi Mansi,

that’s strange. Actually in rev 16720 the important change is that analytics uses Period.getDaysInPeriod(), which adds one day to the result (since period end date is inclusive). See line 331 in DefaultAnalyticsService. Are you sure its not a cache issue? Let me double check on my side.

Lars

Regards,

Mansi Singhal

+91 9900246052

–

On Mon, Sep 15, 2014 at 11:57 AM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars ,

I have taken revision number 16720 from trunk in our branch. What i observed is Number of days in a week are still taken as 6. DateUtilsTest class also assumes number of days as 6.

We were assuming number of days in a week as 7.

Is it a bug or Do I need to take changes from some other revision too.

Regards,

Mansi Singhal

–

Lars · 16 September 2014 06:00

Great.

···

On Tue, Sep 16, 2014 at 7:46 AM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hello,

Thanks Lars for your suggestions. Our branch doesn’t had that method. I have taken those changes and it just works fine.

Actually, commit no: 16720 doesn’t contain that change. Period class might have changed in some other previous commit.

On Mon, Sep 15, 2014 at 7:55 PM, Lars Helge Øverland larshelge@gmail.com wrote:

Hi again,

I have set up an example locally with a few indicators called “x number of cases per day” where numerator is no of cases and denominator is [days] for a week. See attached. It works correctly.

Could it be a build issue?

Can you please check in your branch on Period.java, method getDaysInPeriod, around line 247, and see if one is added to the number of days in period?

public int getDaysInPeriod()

{

Days days = Days.daysBetween( new DateTime( startDate ), new DateTime( endDate ) );

return days.getDays() + 1;

}

Lars

–
Regards,

Mansi Singhal

+91 9900246052

On Mon, Sep 15, 2014 at 2:41 PM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars,

I checked it again clearing cache. Issue still remains the same. Can you please check it once again on your side.

On Mon, Sep 15, 2014 at 5:35 PM, Lars Helge Øverland larshelge@gmail.com wrote:

Hi Mansi,

that’s strange. Actually in rev 16720 the important change is that analytics uses Period.getDaysInPeriod(), which adds one day to the result (since period end date is inclusive). See line 331 in DefaultAnalyticsService. Are you sure its not a cache issue? Let me double check on my side.

Lars

Regards,

Mansi Singhal

+91 9900246052

–

On Mon, Sep 15, 2014 at 11:57 AM, Mansi Singhal msinghal@thoughtworks.com wrote:

Hey Lars ,

I have taken revision number 16720 from trunk in our branch. What i observed is Number of days in a week are still taken as 6. DateUtilsTest class also assumes number of days as 6.

We were assuming number of days in a week as 7.

Is it a bug or Do I need to take changes from some other revision too.

Regards,

Mansi Singhal

–