CLDR-16835 Non-TC locales and extended locales remain open longer #3795

srl295 · 2024-06-11T14:44:16Z

This PR completes the ticket.
New API SubmissionLocales.isTcLocale() and tests:
- root is a TC locale
- Anything listed in TC_ORG_LOCALES is a TC locale, such as, say, fr.
- Any sublocale of a TC locale is a TC locale, such as, say, fr_CA or fr_CM
- Anything else is NOT a TC locale, such as csw.
a new config parameter: CLDR_EXTENDED_PHASE. This may NOT be set in main phase READONLY.
- This new phase only affects locales that are not TC locales, or are listed in the CLDR_EXTENDED_SUBMISSION list.
Internal API to get this phase, taking a CLDRLocale.
UI where the extended phase is shown with a notice, ONLY if it is different from the main phase. No UI change for other users or locales.

Note: The original ticket was to 'automatically' leave DDL open longer, but I have here opted for a 'two tier' approach, where there is an explicit second phase that's applied only to some locales. This way there might be fewer surprises and the TC can choose exactly what phase the extended locales are in rather than have it be computed.

ALLOW_MANY_COMMITS=true

See comments below for example screenshots.

Sample configuration files:

# cldr.properties
CLDR_PHASE=VETTING_CLOSED
CLDR_EXTENDED_PHASE=SUBMIT
CLDR_EXTENDED_SUBMISSION=gsw fr es

Example logfile entries:

Phase: Vetting Closed, CheckCLDR Phase: FINAL_TESTING, Extended Phase: Data Submission
CLDR_EXTENDED_SUBMISSION=gsw fr es

- add CLDR_DDL_PHASE configuration var - add separate DDL phase, defaulting to CLDR_PHASE - pass phase through to client - add separate tc org parameter in the locales list

- includes test - a TC locale includes those whose parents are TC locales.

- add notifier for locMap change - add UI to show special phase for non-TC locales

- add UI to show special phase for non-TC locales

- back end changes supporting a separate ddl phase

srl295 · 2024-06-11T15:05:47Z

In the examples below, the vetter has permissions for both csw (non-tc) and fr (tc)

Example: CLDR_PHASE=VETTING but CLDR_DDL_PHASE=SUBMIT

csw

The yellow highlight has a tooltip, "Note, as a non-TC (DDL) locale, the phase has been extended." Note the plusses and items being added.

fr

Note the phase is here VETTING and the plus is not available.

CLDR_PHASE=VETTING_CLOSED but CLDR_DDL_PHASE=SUBMIT

locale list

Shows as 'Vetting Closed', does NOT show fr but lists csw as clickable.

csw

Same as before.

fr

User is no longer able to vote. Forum remains open.

tools/cldr-apps/js/src/views/MainHeader.vue

tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyMain.java

btangmu · 2024-06-11T16:04:10Z

tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyMain.java

+     * @returns the current phase for the locale. This is the preferred API.
+     */
+    public static CheckCLDR.Phase getCPhase(CLDRLocale loc) {
+        return phase(loc).getCPhase();


"C" for "current" is a little cryptic; I didn't know what it stood for until I got here. How about getCurrentPhase? Or a name that clarifies this method is appropriate for both DDL and non-DDL locales?

It seems like a terminology is needed, distinguishing 3 categories:

methods/data specifically for DDL

methods/data specifically for non-DDL

methods/data that apply to either DDL or non-DDL, depending on the specified locale

There's also the long-standing confusion between CheckCLDR.Phase and SurveyMain.Phase, which still makes my head spin. Of course, fixing that is out of the scope of this ticket, but as long as new methods are being named, it would SO helpful for the new methods/data to use a naming convention that distinguishes them -- maybe checkPhase vs stPhase?

Also, it seems error prone to have three public methods: phase(), phase(locale), and getCPhase(loc). If the no-argument phase() ignores the existence of DDL, what ensures that various modules are consistent in treating DDL differently from non-DDL?

it's not current, it's CheckCLDR. CheckCLDR added its own 'Phase' enum distinct from ST's, and they haven't been fully reconciled yet. It's the "long-standing confusion between CheckCLDR.Phase and SurveyMain.Phase".

I will work on making these more clearer and consistent. Perhaps phase() could be renamed to overallPhase() or something.

And that would need to be overallPhase(locale), right? Without the locale, it can't be overall. So ideally, phase() without a locale parameter might be private? currently it has about 14 callers (before this PR) (not counting jsp)

srl295 · 2024-06-11T17:03:04Z

@btangmu does the basic structure seem OK given the API improvement comments?

macchiati · 2024-06-11T17:17:31Z

I'd suggest changing the ST Phase to be SPhase, and all api's accessing it to also be SPhase.

srl295 · 2024-06-11T17:19:23Z

I'd suggest changing the ST Phase to be SPhase, and all api's accessing it to also be SPhase.

Ideally we'd merge the two together. The problem is that SurveyTool distinguishes between READONLY and VETTING_CLOSED, whereas CheckCLDR doesn't.

btangmu · 2024-06-11T17:29:06Z

I'll approve as a step in the right direction, assuming that DDL users will be better off with it even if it's not fully consistent yet... Have you investigated the callers of phase() without locale? I'm especially concerned that DDL vetters will get mixed results, such as not getting phase=SUBMIT behavior in all circumstances where they're supposed to

btangmu · 2024-06-11T17:30:38Z

tools/cldr-apps/js/src/views/MainHeader.vue

+        <span
+          class="ddlException"
+          v-if="ddlException"
+          title="Note: As a non-TC DDL locale, this phase has been extended."


ideally this title might include ddlPhase, since "extended" is vague -- extended to what?

tools/cldr-apps/src/main/java/org/unicode/cldr/web/SurveyAjax.java

btangmu · 2024-06-11T17:34:57Z

I'd suggest changing the ST Phase to be SPhase, and all api's accessing it to also be SPhase.

The fact that I couldn't tell whether CPhase meant "current phase" or "CLDRCheck phase" points to a general problem with single-letter abbreviations. How about STPhase and CheckPhase, pending the merger Steven suggested?

- replaced phase() and getCPhase() with getSurveyPhase() and toCheckCLDRPhase() for more clarity - renamed the non-locale calls to getOverallSurveyPhase(), and reviewed. See also CLDR-8196 for merging SurveyTool.Phase and CheckCLDR.Phase

srl295 · 2024-06-11T17:39:25Z

I'd suggest changing the ST Phase to be SPhase, and all api's accessing it to also be SPhase.

The fact that I couldn't tell whether CPhase meant "current phase" or "CLDRCheck phase" points to a general problem with single-letter abbreviations. How about STPhase and CheckPhase, pending the merger Steven suggested?

we also have classes such as Level which are used in two different scopes. Anyway, I've updated the API calls for clarity.

btangmu

it's not quite clear what "overall" means or contrasts with...

srl295 · 2024-06-11T19:48:48Z

it's not quite clear what "overall" means or contrasts with...

I'm updating this per today's meeting. But, it should be reviewable as a delta.

macchiati · 2024-06-11T21:38:50Z

How about STPhase and CheckPhase, pending the merger Steven suggested?

SGTM

…

On Tue, Jun 11, 2024 at 10:35 AM Tom Bishop ***@***.***> wrote: I'd suggest changing the ST Phase to be SPhase, and all api's accessing it to also be SPhase. The fact that I couldn't tell whether CPhase meant "current phase" or "CLDRCheck phase" points to a general problem with single-letter abbreviations. How about STPhase and CheckPhase, pending the merger Steven suggested? — Reply to this email directly, view it on GitHub <#3795 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMBFV4BSRZCWOQ7AMVDZG4YNPAVCNFSM6AAAAABJEP65ECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRGI4DKNJWGA> . You are receiving this because your review was requested.Message ID: ***@***.***>

macchiati · 2024-06-11T21:53:56Z

Ideally we'd merge the two together. The problem is that SurveyTool

distinguishes between READONLY and VETTING_CLOSED, whereas CheckCLDR doesn't. I think as far as CLDR is concerned, those would be treated like FINAL_TESTING. If so, then we could introduce those values in CheckCLDR.Phase, and change the call sites for CheckCLDR.Phase.FINAL_TESTING. It appears that there are less than a dozen of those. *if* (Phase.*FINAL_TESTING* == getPhase() || Phase.*BUILD* == getPhase()) { setSkipTest(*false*); // ok turns into *if* (Phase.*FINAL_TESTING* == getPhase() || Phase.*BUILD* == getPhase() || Phase.READONLY == getPhase() | Phase.VETTING_CLOSED == getPhase()) { We could make it even cleaner if we add two methods: 1. isPhase(Set<Phase> testAgainst) 2. isPhase(Phase... testAgainst) That way someone could write: *if* (isPhase(Phase.*FINAL_TESTING, *Phase.*BUILD, *Phase.READONLY, Phase.VETTING_CLOSED)) { and/or we add some static sets for the common cases. It also appears that in many cases, the desired behavior is really to test against whether the ST is open, so cleaner in that case to have: *if* (!isPhase(Phase.*SUBMISSION, *Phase.*VETTING*)) {

…

On Tue, Jun 11, 2024 at 2:38 PM Mark Davis Ⓤ ***@***.***> wrote: > How about STPhase and CheckPhase, pending the merger Steven suggested? SGTM On Tue, Jun 11, 2024 at 10:35 AM Tom Bishop ***@***.***> wrote: > I'd suggest changing the ST Phase to be SPhase, and all api's accessing > it to also be SPhase. > > The fact that I couldn't tell whether CPhase meant "current phase" or > "CLDRCheck phase" points to a general problem with single-letter > abbreviations. How about STPhase and CheckPhase, pending the merger Steven > suggested? > > — > Reply to this email directly, view it on GitHub > <#3795 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACJLEMBFV4BSRZCWOQ7AMVDZG4YNPAVCNFSM6AAAAABJEP65ECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRRGI4DKNJWGA> > . > You are receiving this because your review was requested.Message ID: > ***@***.***> >

- formerly ddlPhase - New configuration parameters: - CLDR_EXTENDED_PHASE - the phase for non-TC and for specific extended locales - CLDR_EXTENDED_SUBMISSION - a separated list of TC locales that are additionally given the extended phase. - Renamed API functions to reflect names such as getExtendedPhase()

btangmu · 2024-06-12T14:14:47Z

tools/cldr-apps/js/src/views/MainHeader.vue

-          title="Note: As a non-TC DDL locale, this phase has been extended."
+          class="extendedException"
+          v-if="extendedException"
+          title="Note: This phase has been extended for this locale."


If TC locales are in Vetting mode, and DDL locales are still in Submission mode, then if a DDL locale is selected, will this show "Vetting (extended)" or "Submission (extended)"? I'm guessing the latter but not quite sure

The latter. And it doesn't have to be a DDL locale, it could be one on the extended list.

- code was still using TC locale instead of extended locale for the phase test

- test needed to be updated

macchiati · 2024-06-12T15:22:09Z

The ! is ok for now. (We can make it clearer later, not blocker for this.)

srl295 · 2024-06-12T16:38:54Z

The ! is ok for now. (We can make it clearer later, not blocker for this.)

I think you meant to comment on #3796

macchiati · 2024-06-12T17:12:03Z

Yes, sorry for mixing the PR comments.

…

On Wed, Jun 12, 2024 at 9:39 AM Steven R. Loomis ***@***.***> wrote: The ! is ok for now. (We can make it clearer later, not blocker for this.) I think you meant to comment on #3796 <#3796> — Reply to this email directly, view it on GitHub <#3795 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACJLEMG2RPBFT3S3THUQ2MLZHB2TJAVCNFSM6AAAAABJEP65ECVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRTGQ3TONRUGQ> . You are receiving this because your review was requested.Message ID: ***@***.***>

macchiati

LGTM

macchiati · 2024-06-13T01:12:58Z

However, I can't speak to the js or vue changes.

srl295 · 2024-06-13T12:58:53Z

Will plan to merge after this current push.

srl295 · 2024-06-13T16:13:18Z

However, I can't speak to the js or vue changes.

Thanks. Tom has reviewd.

srl295 added 5 commits June 10, 2024 18:16

CLDR-16835 prepare for separate DDL phase

a4718a3

- add CLDR_DDL_PHASE configuration var - add separate DDL phase, defaulting to CLDR_PHASE - pass phase through to client - add separate tc org parameter in the locales list

CLDR-16835 add SubmissionLocales.isTcLocale()

8ece119

- includes test - a TC locale includes those whose parents are TC locales.

CLDR-16835 ddlLocale

ef44a9d

- add notifier for locMap change - add UI to show special phase for non-TC locales

CLDR-16835 ddlLocale

c9f8908

- add UI to show special phase for non-TC locales

CLDR-16835 ddl phase - back end changes for separate ddl phase

86af6e4

- back end changes supporting a separate ddl phase

srl295 self-assigned this Jun 11, 2024

srl295 requested review from macchiati, AEApple, stenshamn, pedberg-icu and DraganBesevic June 11, 2024 15:06

srl295 marked this pull request as ready for review June 11, 2024 15:06

CLDR-16835 ddl phase - prettier

44e0b01

srl295 mentioned this pull request Jun 11, 2024

CLDR-16835 DDL announcement #3796

Merged

1 task

btangmu reviewed Jun 11, 2024

View reviewed changes

btangmu previously approved these changes Jun 11, 2024

View reviewed changes

srl295 dismissed btangmu’s stale review via 701415f June 11, 2024 17:36

srl295 requested a review from btangmu June 11, 2024 17:37

btangmu previously approved these changes Jun 11, 2024

View reviewed changes

srl295 dismissed btangmu’s stale review via b90be94 June 12, 2024 13:55

srl295 changed the title ~~CLDR-16835 Non-TC locales remain open longer~~ CLDR-16835 Non-TC locales and extended locales remain open longer Jun 12, 2024

srl295 requested a review from btangmu June 12, 2024 14:00

btangmu previously approved these changes Jun 12, 2024

View reviewed changes

CLDR-16835 Extended Submission Phase - bugfix

ad7d408

- code was still using TC locale instead of extended locale for the phase test

srl295 dismissed btangmu’s stale review via ad7d408 June 12, 2024 14:31

btangmu previously approved these changes Jun 12, 2024

View reviewed changes

CLDR-16835 Extended Submission Phase - bugfix

9c197a8

- test needed to be updated

srl295 dismissed btangmu’s stale review via 9c197a8 June 12, 2024 15:04

srl295 requested a review from btangmu June 12, 2024 15:38

btangmu approved these changes Jun 12, 2024

View reviewed changes

macchiati approved these changes Jun 13, 2024

View reviewed changes

srl295 merged commit 88b9689 into unicode-org:main Jun 13, 2024
11 checks passed

srl295 deleted the cldr-16835/ddl-separate-phase branch June 13, 2024 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLDR-16835 Non-TC locales and extended locales remain open longer #3795

CLDR-16835 Non-TC locales and extended locales remain open longer #3795

srl295 commented Jun 11, 2024 •

edited

Loading

srl295 commented Jun 11, 2024

btangmu Jun 11, 2024

srl295 Jun 11, 2024

srl295 Jun 11, 2024

btangmu Jun 11, 2024

srl295 commented Jun 11, 2024

macchiati commented Jun 11, 2024

srl295 commented Jun 11, 2024

btangmu commented Jun 11, 2024

btangmu Jun 11, 2024

btangmu commented Jun 11, 2024

srl295 commented Jun 11, 2024 •

edited

Loading

btangmu left a comment

srl295 commented Jun 11, 2024

macchiati commented Jun 11, 2024 via email

macchiati commented Jun 11, 2024 via email

btangmu Jun 12, 2024

srl295 Jun 12, 2024

macchiati commented Jun 12, 2024

srl295 commented Jun 12, 2024

macchiati commented Jun 12, 2024 via email

macchiati left a comment

macchiati commented Jun 13, 2024

srl295 commented Jun 13, 2024

srl295 commented Jun 13, 2024

CLDR-16835 Non-TC locales and extended locales remain open longer #3795

CLDR-16835 Non-TC locales and extended locales remain open longer #3795

Conversation

srl295 commented Jun 11, 2024 • edited Loading

srl295 commented Jun 11, 2024

Example: CLDR_PHASE=VETTING but CLDR_DDL_PHASE=SUBMIT

CLDR_PHASE=VETTING_CLOSED but CLDR_DDL_PHASE=SUBMIT

btangmu Jun 11, 2024

Choose a reason for hiding this comment

srl295 Jun 11, 2024

Choose a reason for hiding this comment

srl295 Jun 11, 2024

Choose a reason for hiding this comment

btangmu Jun 11, 2024

Choose a reason for hiding this comment

srl295 commented Jun 11, 2024

macchiati commented Jun 11, 2024

srl295 commented Jun 11, 2024

btangmu commented Jun 11, 2024

btangmu Jun 11, 2024

Choose a reason for hiding this comment

btangmu commented Jun 11, 2024

srl295 commented Jun 11, 2024 • edited Loading

btangmu left a comment

Choose a reason for hiding this comment

srl295 commented Jun 11, 2024

macchiati commented Jun 11, 2024 via email

macchiati commented Jun 11, 2024 via email

btangmu Jun 12, 2024

Choose a reason for hiding this comment

srl295 Jun 12, 2024

Choose a reason for hiding this comment

macchiati commented Jun 12, 2024

srl295 commented Jun 12, 2024

macchiati commented Jun 12, 2024 via email

macchiati left a comment

Choose a reason for hiding this comment

macchiati commented Jun 13, 2024

srl295 commented Jun 13, 2024

srl295 commented Jun 13, 2024

srl295 commented Jun 11, 2024 •

edited

Loading

srl295 commented Jun 11, 2024 •

edited

Loading