Repository: moment/luxon (js)

Issue:
Duration.toISOTime has locale-dependent output (using ar-QA locale)
**Describe the bug**
We ran into a production error today trying to parse an ISO time passed from the client to the server. The client had an Arabic locale and for some reason the locale-formatted time value was making its way to the server and crashing the ISO parse.

**To Reproduce**
I eventually boiled down the problem to this repro (running in the console at https://moment.github.io/luxon) :
```javascript
> luxon.Settings.defaultLocale = "ar-QA"
'ar-QA'
> luxon.Duration.fromObject({hours: 1, minutes: 4}).toISOTime()
'٠١:٠٤:٠٠.٠٠٠'
> luxon.DateTime.now().toISOTime()
'16:53:34.372-04:00'
> luxon.Duration.fromISOTime(luxon.Duration.fromObject({hours: 1, minutes: 1}).toISOTime()).invalidExplanation
`the input "٠١:٠١:٠٠.٠٠٠" can't be parsed as ISO 8601`
```

**Actual vs Expected behavior**
`luxon.Duration.fromObject({hours: 1, minutes: 4}).toISOTime()` outputs `٠١:٠٤:٠٠.٠٠٠` in certain locales.
I would have expected the toISOTime to be locale-independent and always output `01:04:00.000`

If I'm misunderstanding the ISO standard in this case, I would at least expect the roundtrip not to fail:
```javascript
> luxon.Duration.fromISOTime(luxon.Duration.fromObject({hours: 1, minutes: 1}).toISOTime()).invalidExplanation
`the input "٠١:٠١:٠٠.٠٠٠" can't be parsed as ISO 8601`
```

**Desktop:**

- OS: Windows, Mac
- Browser: Chrome 111
- Luxon version: 3.3.0
- Your timezone: "America/New_York"


Pull request:
Fix Duration#toISOTime depending on the locale
I have created this as a pull request, because _technically_ this is a breaking change.
Previously `DateTime#toISOTime` would ignore milliseconds even if only `suppressSeconds` was set. `Duration#toISOTime` did not do so. I have aligned the behavior between the two, so that no data is ever silently ignored (suppressing only works if _all_ hidden units are 0).

Please let me know if you think preserving the original behavior of suppressSeconds on DateTime is worthwhile. 

Fixes #1403 

Patch:
diff --git a/src/datetime.js b/src/datetime.js
index 5120b056..0e3c4eee 100644
--- a/src/datetime.js
+++ b/src/datetime.js
@@ -224,14 +224,14 @@ function toISOTime(
   if (extended) {
     c += ":";
     c += padStart(o.c.minute);
-    if (o.c.second !== 0 || !suppressSeconds) {
+    if (o.c.millisecond !== 0 || o.c.second !== 0 || !suppressSeconds) {
       c += ":";
     }
   } else {
     c += padStart(o.c.minute);
   }
 
-  if (o.c.second !== 0 || !suppressSeconds) {
+  if (o.c.millisecond !== 0 || o.c.second !== 0 || !suppressSeconds) {
     c += padStart(o.c.second);
 
     if (o.c.millisecond !== 0 || !suppressMilliseconds) {
diff --git a/src/duration.js b/src/duration.js
index fc512b6e..72f67fb4 100644
--- a/src/duration.js
+++ b/src/duration.js
@@ -12,6 +12,7 @@ import {
   roundTo,
 } from "./impl/util.js";
 import Settings from "./settings.js";
+import DateTime from "./datetime.js";
 
 const INVALID = "Invalid Duration";
 
@@ -548,26 +549,11 @@ export default class Duration {
       includePrefix: false,
       format: "extended",
       ...opts,
+      includeOffset: false,
     };
 
-    const value = this.shiftTo("hours", "minutes", "seconds", "milliseconds");
-
-    let fmt = opts.format === "basic" ? "hhmm" : "hh:mm";
-
-    if (!opts.suppressSeconds || value.seconds !== 0 || value.milliseconds !== 0) {
-      fmt += opts.format === "basic" ? "ss" : ":ss";
-      if (!opts.suppressMilliseconds || value.milliseconds !== 0) {
-        fmt += ".SSS";
-      }
-    }
-
-    let str = value.toFormat(fmt);
-
-    if (opts.includePrefix) {
-      str = "T" + str;
-    }
-
-    return str;
+    const dateTime = DateTime.fromMillis(millis, { zone: "UTC" });
+    return dateTime.toISOTime(opts);
   }
 
   /**


Test patch:
diff --git a/test/datetime/format.test.js b/test/datetime/format.test.js
index 3f584444..2aeca906 100644
--- a/test/datetime/format.test.js
+++ b/test/datetime/format.test.js
@@ -96,6 +96,12 @@ test("DateTime#toISO() suppresses [milli]seconds", () => {
   const noZeroSeconds = { suppressSeconds: true, suppressMilliseconds: true };
   expect(dt.set({ millisecond: 0 }).toISO(noZeroSeconds)).toBe("1982-05-25T09:23:54Z");
   expect(dt.set({ seconds: 0, milliseconds: 0 }).toISO(noZeroSeconds)).toBe("1982-05-25T09:23Z");
+
+  const suppressOnlySeconds = { suppressSeconds: true };
+  expect(dt.set({ seconds: 0 }).toISO(suppressOnlySeconds)).toBe("1982-05-25T09:23:00.123Z");
+  expect(dt.set({ seconds: 0, milliseconds: 0 }).toISO(suppressOnlySeconds)).toBe(
+    "1982-05-25T09:23Z"
+  );
 });
 
 test("DateTime#toISO() returns null for invalid DateTimes", () => {
diff --git a/test/duration/format.test.js b/test/duration/format.test.js
index abcf4f4c..408660f8 100644
--- a/test/duration/format.test.js
+++ b/test/duration/format.test.js
@@ -117,6 +117,12 @@ test("Duration#toISOTime returns null if the value is outside the range of one d
   expect(Duration.fromObject({ milliseconds: -1 }).toISOTime()).toBe(null);
 });
 
+test("Duration#toISOTime is not influenced by the locale", () => {
+  expect(Duration.fromObject({ hours: 3, minutes: 10 }, { locale: "ar-QA" }).toISOTime()).toBe(
+    "03:10:00.000"
+  );
+});
+
 //------
 // #toMillis()
 //------