joniles / mpxj

Primary repository for MPXJ library
http://www.mpxj.org/
GNU Lesser General Public License v2.1
248 stars 104 forks source link

PrimaveraPMFileReader: Date/time format and Calendar/StandardWorkWeek/WorkTime ranges #579

Closed alex-matatov closed 1 year ago

alex-matatov commented 1 year ago

Hi Jon,

There are 2 issues that we faced in reading P6/XML files.

  1. We got a P6 xml file (V18.8) where date/time are without seconds like that: <PlannedStartDate>2020-02-15T00:00</PlannedStartDate>. So, PrimaveraPMFileReader can't parse them because it expects date/time with seconds.

I think that MPXJ lib could handle this situation by using 2 date/time formatters. If first one fails then second one could try to parse.

I drafted the fix in the attached net.sf.mpxj.primavera.DatatypeConverter file (see parseDateTime() method). DatatypeConverter.java.txt

What do you think ? Does it make sense ?

  1. PrimaveraPMFileReader.getEndTime() method is used to parse "Calendar/StandardWorkWeek/StandardWorkHours/WorkTime/Finish" entities:
calendarHours.add(new LocalTimeRange(work.getStart(), getEndTime(work.getFinish())));

This method always adds 1 minute to the day. So when P6/xml has section like below FinishDate will be 17:00:00. It is Ok and your comment above the method explains why.

<WorkTime>
   <Start>09:00:00</Start>
   <Finish>16:59:59</Finish>
</WorkTime>

However, we got a P6 xml file (V18.8) where WorkTime is defined like below. So //WorkTime[0].FinishDate will be 17:01:00 and //WorkTime[1].StartDate is 17:00:00. So we have overlap here.

 <WorkTime>
    <Start>09:00:00</Start>
    <Finish>17:00:00</Finish>
 </WorkTime>
 <WorkTime>
   <Start>17:00:00</Start>
   <Finish>19:00:00</Finish>
 </WorkTime>

It is not a big deal to handle this case in our app code. However, I think it would be useful to change PrimaveraPMFileReader.getEndTime() like that:

private LocalTime getEndTime(LocalTime date)
   {
      return (date.getMinute() == 0) ?
         date.truncatedTo(ChronoUnit.SECONDS) : // Finish date is aligned to hour - 17:00:00. Let's return it as it
         date.truncatedTo(ChronoUnit.SECONDS).plusMinutes(1); // Finish date is not aligned to hour - 16:59:59. Let's shift it to 1 minute forward
   }

What do you think about that ?

JanecekPetr commented 1 year ago

Hello, Alex's colleague here!

It turns out the P6/XML files are actually both coming from our own software.

Therefore, the irregular datetime format is something we produced. We'll check the schema and fix the issue if the date-time is exactly specced to always contain seconds.

The latter issue with overlapping WorkTimes was a bug we have already fixed. The file was apparently generated before that.

Let us sync on Monday (holiday until then) and get back to you to see whether there's actually something to do here.

joniles commented 1 year ago

@JanecekPetr thanks for the update. Enjoy the break

alex-matatov commented 1 year ago

Hi Jon,

Indeed, the 2 issues above are from our software. So the ticket can be closed and I am sorry for bothering you.

By the way, as a just small improvement of net.sf.mpxj.primavera.DatatypeConverter I would propose to use pre-defined java's ISO DateTimeFormater instead of custom ones:

// private static final DateTimeFormatter DATE_FORMAT = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss");
// private static final DateTimeFormatter TIME_FORMAT = DateTimeFormatter.ofPattern("HH:mm:ss");

private static final DateTimeFormatter DATE_FORMAT = DateTimeFormatter.ISO_LOCAL_DATE_TIME;
private static final DateTimeFormatter TIME_FORMAT = DateTimeFormatter.ISO_LOCAL_TIME;
joniles commented 1 year ago

Thanks for the note regarding ISO_LOCAL_DATE_TIME and ISO_LOCAL_TIME. I'm going to leave the explicit formats used by MPXJ currently as the predefined formats are actually slightly different (they include an optional nanoseconds item).