[Unicode]   Common Locale Data Repository : Bug Tracking Home | Site Map | Search
 
Modify

CLDR Ticket #11103(reviewing)

Opened 10 months ago

Last modified 3 weeks ago

Vote Status information for sub locales are carried over from the main locale

Reported by: kristi Owned by: tbishop
Component: surveytool-other Data Locale:
Phase: dsub Review: mark
Weeks: Data Xpath:
Xref:

Description

It looks like the voting information (the check marks indicating Approved,Contributed, etc..) are getting carried down to sub locales from main locales for those that have not

It's difficult to tell which are locales that people vetted on vs. those that are just getting the inherited.

For example see ko_KP which I'm fairly sure we haven't had any contributors.
another example is ar_MR which I'm also pretty certain that we haven't had any contributors.

In both cases they are all inherited values and show the same voting status as the main locale.
http://st.unicode.org/cldr-apps/v#/ar_MR/Gregorian/1d02739166125786
http://st.unicode.org/cldr-apps/v#/ko_KP/Gregorian/3adf0c8c69672074

Attachments

fr_CA examples with Missing status.JPG (50.3 KB) - added by kristi 10 months ago.
sub-locale data with X (missing) while the main locale has data

Change History

comment:1 Changed 10 months ago by mark

  • Owner changed from anybody to mark
  • Status changed from new to accepted
  • Milestone changed from UNSCH to 34

take a look

comment:2 Changed 10 months ago by kristi

Found some exceptions to what's happening.
These emoji entries are not inheriting the vote status from fr:
http://cldrubs14-test1.cloudapp.net/smoketest/v#/fr_CA/Food_Drink/14c5d3f5c66dd5c6

Changed 10 months ago by kristi

sub-locale data with X (missing) while the main locale has data

comment:3 Changed 10 months ago by mark

  • Component changed from unknown to survey

comment:4 Changed 10 months ago by mark

  • type changed from unknown to survey

comment:5 Changed 8 months ago by kristi

Some locales like ar-SA when all are inherited with no actual votes, it choses the check.
http://st.unicode.org/cldr-apps/v#/ar_SA/Hebrew/1a4cc4c92cb3c872

But in Maori for example, for values where there are no actual votes, it shows the X.http://st.unicode.org/cldr-apps/v#/mi/Gregorian/74f6f850b98466a2

comment:6 Changed 7 months ago by tbishop

  • Owner changed from mark to tbishop

comment:7 Changed 6 months ago by tbishop

  • Keywords inheritance added
  • Milestone changed from 34 to 35

comment:8 Changed 6 months ago by tbishop

For the first example

http://st.unicode.org/cldr-apps/v#/ar_MR/Gregorian/1d02739166125786

there is currently a green check mark in the A column, with tooltip "Status: Approved". What would be the correct status? Intuitively, given no votes in the sublocale, isn't inheritance automatic, and in some sense automatically "approved" if the inherited value was approved in the parent locale?

comment:9 Changed 5 months ago by kristi

For implementation design, see Vote design doc section "Check Mark (Voting Status) behavior"

"if the item is inherited, but not from root, and in the source locale has a value with enough votes, then the icon is a new icon ⇧ (orange color). (add this to TOC-Icons after implementation)
Otherwise it is an X (“does not have enough votes” as in TOC-Icons)"

comment:10 Changed 5 months ago by kristi

  • Priority changed from assess to major

comment:11 Changed 4 months ago by mark

  • Priority changed from major to critical

comment:12 Changed 4 months ago by tbishop

The icon is based on the value of winningStatus, sent from the server to the client as voteResolver.winningStatus in SurveyAjax.JSONWriter.wrap(VoteResolver):

        public static JSONObject wrap(final VoteResolver<String> r) throws JSONException {
            JSONObject ret = new JSONObject().put("raw", r.toString()).put("isDisputed", r.isDisputed())
                .put("lastReleaseStatus", r.getLastReleaseStatus())
                .put("winningValue", r.getWinningValue())
                .put("lastReleaseValue", r.getLastReleaseValue())
                .put("requiredVotes", r.getRequiredVotes())
                .put("winningStatus", r.getWinningStatus());

getWinningStatus is in VoteResolver.java:

    public Status getWinningStatus() {
        if (!resolved) {
            resolveVotes();
        }
        return winningStatus;
    }

resolveVotes starts like this:

    private void resolveVotes() {
        resolved = true;
        // get the votes for each organization
        valuesWithSameVotes.clear();
        totals = organizationToValueAndVote.getTotals(conflictedOrganizations);
        /* Note: getKeysetSortedByCount actually returns a LinkedHashSet, "with predictable iteration order". */
        final Set<T> sortedValues = totals.getKeysetSortedByCount(false, votesThenUcaCollator);
        if (DEBUG) {
            System.out.println("sortedValues :" + sortedValues.toString());
        }
        // if there are no (unconflicted) votes, return lastRelease
        if (sortedValues.size() == 0) {
            if (trunkStatus != null && (lastReleaseStatus == null || trunkStatus.compareTo(lastReleaseStatus) >= 0)) {
                winningStatus = trunkStatus;
                winningValue = trunkValue;
            } else {
                winningStatus = lastReleaseStatus;
                winningValue = lastReleaseValue;
            }
            valuesWithSameVotes.add(winningValue); // may be null
            return;
        }

comment:13 Changed 4 months ago by tbishop

I'm using http://localhost:8080/cldr-apps/v#/ar_MR/Gregorian/1d02739166125786 as the URL for testing. It shows a green checkmark for the inherited winning value, and there are no votes. resolveVotes sets winningStatus = lastReleaseStatus. Prior to that, the constructor for DataRow does:

           VoteResolver<String> resolver = ballotBox.getResolver(xpath);

getResolver (for ar_MR) calls ... getResolverInternal which calls:

    r.setLastRelease(lastValue, lastValue == null ? Status.missing : lastStatus); /* add the last release value */

with lastValue = إثنين and lastStatus = approved

    public void setLastRelease(T lastReleaseValue, Status lastReleaseStatus) {
        this.lastReleaseValue = lastReleaseValue;
        this.lastReleaseStatus = lastReleaseStatus == null ? Status.missing : lastReleaseStatus;
Last edited 4 months ago by tbishop (previous) (diff)

comment:14 Changed 4 months ago by tbishop

It appears that the status we're generally seeing is from the last release, not from the source for inheritance. The value is marked both with a star for last release and with blue background for inheritance. Coincidentally the inherited value and the last release value are the same, as is often the case. Nevertheless, the item is winning because it's last release, not because it's inherited.

Last edited 4 months ago by tbishop (previous) (diff)

comment:15 Changed 4 months ago by tbishop

In STFactory.getResolverInternal:

            final Status lastStatus = getStatus(anOldFile, path, lastValue);

getStatus returns Status.approved:

        private Status getStatus(CLDRFile anOldFile, String path, final String lastValue) {
            Status lastStatus;
            {
                XPathParts xpp = new XPathParts(null, null);
                String fullXPath = anOldFile.getFullXPath(path);
                if (fullXPath == null)
                    fullXPath = path; // throw new
                // InternalError("null full xpath for " +
                // path);
                xpp.set(fullXPath);
                String draft = xpp.getAttributeValue(-1, LDMLConstants.DRAFT);
                lastStatus = draft == null ? Status.approved : VoteResolver.Status.fromString(draft);

There we have draft == null.

comment:16 Changed 4 months ago by tbishop

This URL shows an orange checkmark: http://localhost:8080/cldr-apps/v#/ko_KP/Gregorian/3adf0c8c69672074

This time draft isn't null, and we get Status.contributed here:

   lastStatus = draft == null ? Status.approved : VoteResolver.Status.fromString(draft);

comment:17 Changed 4 months ago by tbishop

I made a branch (branches/tbishop/t11103_orange_arrow) and committed some basic changes for a first approximation. There's one new icon, "inherited.png", and orange up-arrow. There's one new Status value in VoteResolver, "inherited".

There are two places in VoteResolver where winningStatus gets "inherited" if winningValue is INHERITANCE_MARKER:

    private void resolveVotes() {
 ...
        // if there are no (unconflicted) votes, return lastRelease
        if (sortedValues.size() == 0) {
            if (trunkStatus != null && (lastReleaseStatus == null || trunkStatus.compareTo(lastReleaseStatus) >= 0)) {
                winningStatus = trunkStatus;
                winningValue = trunkValue;
            } else {
                winningStatus = lastReleaseStatus;
                if (CldrUtility.INHERITANCE_MARKER.equals(lastReleaseValue)) {
                    winningStatus = Status.inherited;
                }
                winningValue = lastReleaseValue;
            }
...

and

    private Status computeStatus(long weight1, long weight2, Status oldStatus) {
        if (CldrUtility.INHERITANCE_MARKER.equals(winningValue)) {
            return Status.inherited;
        }
        int orgCount = organizationToValueAndVote.getOrgCount(winningValue);
        return weight1 > weight2 &&
            (weight1 >= requiredVotes) ? Status.approved
                : weight1 > weight2 &&
                    (weight1 >= 4 && Status.contributed.compareTo(oldStatus) > 0
                        || weight1 >= 2 && orgCount >= 2) ? Status.contributed
                            : weight1 >= weight2 && weight1 >= 2 ? Status.provisional
                                : Status.unconfirmed;

I expect those aren't quite the right conditions for display of the new icon. Also, we still have some design decisions, whether to have more than one new icon, maybe different colors corresponding to the status in the parent locale. Also there are cases in which winningValue is determined, not by VoteResolver (which returns null) but by DataRow.fixWinningValue(); ideally that code, or a replacement for it, belongs in VoteResolver.

comment:18 Changed 4 months ago by tbishop

Committed changes to trunk with revision [14691]. Different from last version in branch: orange up-arrow is shown if winning item is inherited and doesn’t have the votes for a checkmark.

comment:19 Changed 4 months ago by tbishop

I forgot to include TestSTFactory.xml which needs one change from "provisional" to "inherited".

That's in revision [14692].

comment:20 Changed 4 months ago by tbishop

I’ve tested on smoketest. The only unexpected result is (3), and I believe it’s due to different voting data on smoketest (on which it currently shows zero votes, for v35) from my current localhost (on which it still has votes, for v34). In fact, if I vote for inheritance on smoketest, then it does change to a green check.

Current results on smoketest are:

(1) http://cldr-smoke.unicode.org/smoketest/v#/ar_MR/Gregorian/62c54e2eae198430 - green check

(2) http://cldr-smoke.unicode.org/smoketest/v#/ko_KP/Gregorian/3adf0c8c69672074 - orange up-arrow

(3) http://cldr-smoke.unicode.org/smoketest/v#/fr_CA/T_Europe/5f98c60333499cae - orange up-arrow

(4) http://cldr-smoke.unicode.org/smoketest/v#/fr_CA/T_Europe/2b8dc7b7161b41be - black X

comment:21 Changed 4 months ago by markus

  • Summary changed from Vote Status informaiton for sub locales are carried over from the main locale to Vote Status information for sub locales are carried over from the main locale

comment:22 Changed 4 months ago by tbishop

  • Status changed from accepted to reviewing
  • Review set to mark

comment:23 Changed 2 months ago by tbishop

Orange up-arrow has been temporarily disabled, see changeset [14738] for ticket 11721.

See also ticket 11766.

comment:24 Changed 2 months ago by tbishop

We added “inherited” as one of the Status values in VoteResolver.java. It wasn't clear this would eventually trigger an exception, due to an expectation for “inherited” to be one of the DraftStatus values as well:

VoteResolver.java:

    public enum Status {
        missing, unconfirmed, provisional, contributed, inherited, approved;

CLDRFile.java:

    public enum DraftStatus {
        unconfirmed, provisional, contributed, approved;

The Status value gets turned into a string here in STFactory.java:

        public VoteResolver<String> setValueFromResolver(String path, VoteResolver<String> resolver) {
...
                Status win = resolver.getWinningStatus();
                if (win == Status.approved) {
                    fullPath = baseXPath;
                } else {
                    fullPath = baseXPath + "[@draft=\"" + win + "\"]";
                }

Then that string gets interpreted as DraftStatus here in CheckLogicalGroupings.java:

            DraftStatus draftStatus = DraftStatus.forString(parts.findFirstAttributeValue("draft"));

comment:25 Changed 2 months ago by tbishop

We need to map from VoteResolver.Status (including “inherited”) to CLDRFile.DraftStatus (which does not and should not include “inherited”); “inherited” needs to be mapped to either “provisional” or “unconfirmed” depending on votes.

Maybe should distinguish “inherited-provisional” = orange up-arrow and “inherited-unconfirmed” = red up-arrow? Alternatively, all could be done on the client: when winning value is INHERITANCE_MARKER, replace orange/red X with orange/red up-arrow?

Last edited 7 weeks ago by tbishop (previous) (diff)

comment:26 Changed 7 weeks ago by tbishop

Two possible solutions:

(1) on server, VoteResolver.Status.inherited_provisional = orange up-arrow and VoteResolver.Statusinherited_unconfirmed = red up-arrow; in setValueFromResolver, use a new function explicit function to map VoteResolver.Status to DraftStatus, mapping VoteResolver.Status.inherited_provisional to DraftStatus.provisional and VoteResolver.Status.inherited_unconfirmed to DraftStatus.unconfirmed

(2) on server, no "inherited" VoteResolver.Status; on client, if winning value is INHERITANCE_MARKER, replace orange/red X with orange/red up-arrow

If we don't want to make a visible distinction between orange up-arrow and red up-arrow, we can just use orange for both, while with solution (1) we'd still make the distinction internally, or with solution (2) there would be no need for the distinction.

Argument for (1): server, not client, is responsible for data, including approval status.

Argument for (2): the data isn't really different, this is just a question of how to display the data, which is the client's responsibility. In the special case where the winning value is inherited, we change orange/red X to orange (/red?) arrow.

Last edited 7 weeks ago by tbishop (previous) (diff)

comment:27 Changed 7 weeks ago by tbishop

Even if we go with solution (2), we should still have an explicit function on the server for mapping from VoteResolver.Status to CLDRFile.DraftStatus, and add comments for both enum types explaining the need to keep them compatible.

comment:28 Changed 7 weeks ago by tbishop

Where to put the function that maps VoteResolver.Status to CLDRFile.DraftStatus? The objects currently don't refer to each other. Maybe the function should be in STFactory.java along with setValueFromResolver, which will be its only caller.

comment:29 Changed 7 weeks ago by tbishop

We could revise setValueFromResolver as follows:

...
                if (win == Status.approved) {
                    fullPath = baseXPath;
                } else {
                    DraftStatus draftStatus = draftStatusFromWinningStatus(win);
                    fullPath = baseXPath + "[@draft=\"" + draftStatus.toString() + "\"]";
                }
...

and add this function after it:

        /**
         * Map the given VoteResolver.Status to a CLDRFile.DraftStatus
         *
         * @param win the VoteResolver.Status (winning status)
         * @return the DraftStatus
         *
         * As a rule, the name of each VoteResolver.Status is also a name of a DraftStatus.
         * Any exceptions to that rule should be handled explicitly in this function.
         * 
         * Reference: https://unicode.org/cldr/trac/ticket/11103
         */
        private DraftStatus draftStatusFromWinningStatus(VoteResolver.Status win) {
            try {
                DraftStatus draftStatus = DraftStatus.forString(win.toString());
                return draftStatus;
            } catch (IllegalArgumentException e) {
                SurveyLog.logException(e, "Exception in draftStatusFromWinningStatus of " + win);
                return DraftStatus.unconfirmed;
            }
        }

That's enough for solution (2). For solution (1), we would need to add the explicit mappings from VoteResolver.Status.inherited_provisional to DraftStatus.provisional and from VoteResolver.Status.inherited_unconfirmed to DraftStatus.unconfirmed.

comment:30 Changed 3 weeks ago by tbishop

See ticket 11766 for remainder of this task.

View

Add a comment

Modify Ticket

Action
as reviewing
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.