Merge pull request #421 from ical4j/bugfix/temporal-amount-adapter

Fix minutes/seconds parsing

src/main/java/net/fortuna/ical4j/model/TemporalAmountAdapter.java

@@ -7,7 +7,10 @@
 import java.io.Serializable;
 import java.time.Duration;
 import java.time.Instant;
+import java.time.LocalDateTime;
 import java.time.Period;
+import java.time.temporal.ChronoUnit;
+import java.time.temporal.Temporal;
 import java.time.temporal.TemporalAmount;
 import java.util.Date;
 
@@ -28,11 +31,14 @@ public TemporalAmount getDuration() {
 
     @Override
     public String toString() {
-        String retVal = null;
+        return toString(LocalDateTime.now());
+    }
+    public String toString(Temporal seed) {
+        String retVal;
         if (Duration.ZERO.equals(duration) || Period.ZERO.equals(duration)) {
             retVal = duration.toString();
         } else if (duration instanceof Period) {
-            retVal = periodToString(((Period) duration).normalized());
+            retVal = periodToString(((Period) duration).normalized(), seed);
         } else {
             retVal = durationToString((Duration) duration);
         }
@@ -46,22 +52,22 @@ public String toString() {
      * @param period a period instance
      * @return a string representation of the period that is compliant with the RFC5545 specification.
      */
-    private String periodToString(Period period) {
+    private String periodToString(Period period, Temporal seed) {
         String retVal;
-        Period absPeriod = period.isNegative() ? period.negated() : period;
-        if (absPeriod.getYears() != 0) {
-            int weeks = absPeriod.getYears() * 52;
+        Temporal adjustedSeed = seed.plus(period);
+        if (period.getYears() != 0) {
+            long weeks = Math.abs(seed.until(adjustedSeed, ChronoUnit.WEEKS));
             retVal = String.format("P%dW", weeks);
-        } else if (absPeriod.getMonths() != 0) {
-            int weeks = absPeriod.getMonths() * 4;
+        } else if (period.getMonths() != 0) {
+            long weeks = Math.abs(seed.until(adjustedSeed, ChronoUnit.WEEKS));
             retVal = String.format("P%dW", weeks);
-        } else if (absPeriod.getDays() % 7 == 0) {
-            int weeks = absPeriod.getDays() / 7;
+        } else if (period.getDays() % 7 == 0) {
+            long weeks = Math.abs(seed.until(adjustedSeed, ChronoUnit.WEEKS));
             retVal = String.format("P%dW", weeks);
         } else {
-            retVal = absPeriod.toString();
+            retVal = period.toString();
         }
-        if (period.isNegative()) {
+        if (period.isNegative() && !retVal.startsWith("-")) {
             return "-" + retVal;
         } else {
             return retVal;
@@ -92,11 +98,17 @@ private String durationToString(Duration duration) {
                 if (hours > 0) {
                     if (seconds > 0) {
                         retVal = String.format("P%dDT%dH%dM%dS", days, hours, minutes, seconds);
+                    } else if (minutes > 0) {
+                        retVal = String.format("P%dDT%dH%dM", days, hours, minutes);
                     } else {
                         retVal = String.format("P%dDT%dH", days, hours);
                     }
                 } else if (minutes > 0) {
-                    retVal = String.format("P%dDT%dM", days, minutes);
+                    if (seconds > 0) {
+                        retVal = String.format("P%dDT%dM%dS", days, minutes, seconds);
+                    } else {
+                        retVal = String.format("P%dDT%dM", days, minutes);
+                    }
                 } else if (seconds > 0) {
                     retVal = String.format("P%dDT%dS", days, seconds);
                 }

src/test/groovy/net/fortuna/ical4j/model/TemporalAmountAdapterTest.groovy

@@ -1,6 +1,7 @@
 package net.fortuna.ical4j.model
 
 import net.fortuna.ical4j.util.CompatibilityHints
+import spock.lang.Ignore
 import spock.lang.Specification
 import spock.lang.Unroll
 
@@ -17,13 +18,16 @@ class TemporalAmountAdapterTest extends Specification {
         Duration.ofHours(4)         | "PT4H"
         Duration.ofHours(-4)         | "-PT4H"
         Duration.ofDays(12)         | "P12D"
+        Duration.ofDays(12).plusMinutes(30)     | "P12DT30M"
+        Duration.ofDays(12).plusMinutes(-30)    | "P11DT23H30M"
+        Duration.ofDays(-12).plusMinutes(-30)    | "-P12DT30M"
         java.time.Period.ofDays(12) | "P12D"
         java.time.Period.ofWeeks(7) | "P7W"
         java.time.Period.ofDays(365) | "P365D"
         java.time.Period.ofDays(364) | "P52W"
         java.time.Period.ofYears(1) | "P52W"
-        java.time.Period.ofMonths(6) | "P24W"
-        java.time.Period.ofMonths(-6) | "-P24W"
+        java.time.Period.ofMonths(6) | "P26W"
+        java.time.Period.ofMonths(-6) | "-P26W"
         Duration.ofDays(15).plusHours(5).plusSeconds(20)    | 'P15DT5H0M20S'
     }
 
@@ -181,4 +185,21 @@ class TemporalAmountAdapterTest extends Specification {
         adapter1.duration == -adapter2.duration
     }
 
+    def 'testTemporalAmountAdapter_durationToString_DropsMinutes'() {
+        expect: "P1DT1H4M" == TemporalAmountAdapter.parse("P1DT1H4M") as String
+    }
+
+    @Ignore
+    def 'testTemporalAmountAdapter_Months'() {
+        // https://github.com/ical4j/ical4j/issues/419
+        // A month usually doesn't have 4 weeks = 4*7 days = 28 days (except February in non-leap years).
+        expect: "P4W" != new TemporalAmountAdapter(java.time.Period.ofMonths(1)) as String
+    }
+
+    @Ignore
+    def 'testTemporalAmountAdapter_Year'() {
+        // https://github.com/ical4j/ical4j/issues/419
+        // A year has 365 or 366 days, but never 52 weeks = 52*7 days = 364 days.
+        expect: "P52W" != new TemporalAmountAdapter(java.time.Period.ofYears(1)) as String
+    }
 }