fix The Korean sentence order in Authorship description

[tandara0]

This commit is about the issue: https://github.com/OHDSI/WebAPI/issues/2381 And the commit involved in WebAPI: https://github.com/OHDSI/WebAPI/pull/2382

[chrisknoll]

Is there any way you can combine those multiple elements into one templated field like:

 "createdLabel": "<%=createdBy%>가  경에 생성 <%=createdDate%>",

Such that we create one element to contain the full sentence of what the created by message shoudl be (and separately, the modified one).

This is the code:

<span data-bind="text: createdText"></span>
    <span data-bind="visible: createdBy, text: ko.i18nformat('components.authorship.byCreated', 'by <%=createdBy%>', {createdBy: createdBy})"></span>
    <span data-bind="text: ko.i18n('components.authorship.on', 'on')"></span>
    <span data-bind="text: createdDate"></span>
    <span data-bind="text: ko.i18n('components.authorship.createdEnd', ' ')"></span><span data-bind="visible: modifiedDate">,</span>
    <span data-bind="visible: modifiedDate">
        <span data-bind="text: ko.i18n('components.authorship.modified', 'modified')"></span>
        <span data-bind="visible: createdBy, text: ko.i18nformat('components.authorship.byModified', 'by <%=modifiedBy%>', {modifiedBy: modifiedBy})"></span>
        <span data-bind="text: ko.i18n('components.authorship.on', 'on')"></span>
        <span data-bind="text: modifiedDate"></span>
        <span data-bind="text: ko.i18n('components.authorship.modifiedEnd', ' ')"></span>
    </span>

The issue is the content of the message is being broken apart by multiple spans and databinds (and multiple i18n tokens). Would it be possible to restructure this into just 2 spans with 2 i18n tokens (components.authorship.createdLabel and components.authorship.modifiedLabel), and allt he above spans turn into something like:

<span data-bind="visible: createdBy, 
  text: ko.i18nformat('components.authorship.createdLabel', 
                                  'by <%=createdBy%> on <%=cratedDate%>', 
                                  {createdBy: createdBy, createdDate: createdDate})"></span>
<span data-bind="visible: modifiedBy, 
  text: ko.i18nformat('components.authorship.modifiedLabel', 
                                  'by <%=modifiedBy%> on <%=modifiedDate%>', 
                                  {createdBy: createdBy, createdDate: createdDate})"></span>

This would solve the problem of language structure by encapsulating the entire sentence into one block.

Would this be possible for you to do? I think I'd prefer a solution that reduces the number of I18N tokens in our resources instead of adding new ones that only apply in certain contexts.

[tandara0]

Dear @chrisknoll,

I agree to reduce the elements of the authorship components. If I understand what you said, the authorship components of english will be changed like:

before:

    "authorship": {
      "created": "created",
      "versionCreated": "version created",
      "modified": "modified",
      "on": "on",
      "byCreated": "by <%=createdBy%>",
      "byModified": "by <%=modifiedBy%>"
    },

after:

    "authorship": {
      "createdLabel": "created by <%=createdBy%> on <%=createdDate%>",
      "modifiedLabel": "modified by <%=modifiedBy%> on <%=modifiedDate%>"
    },

this change will be applied the file messages_en.json in OHDSI/WebAPI git. and consequently the other 3 languages json will be changed like:

messages_ko.json:

    "authorship": {
      "createdLabel": "<%=createdBy%>가 <%=createdDate%> 경에 생성",
      "modifiedLabel": "<%=modifiedBy%>가 <%=modifiedDate%> 경에 수정"
    },

messages_ru.json:

    "authorship": {
      "createdLabel": "создано: <%=createdBy%> <%=createdDate%>",
      "modifiedLabel": "изменено: <%=modifiedBy%> <%=modifiedDate%>"
    },

messages_zh.json:

    "authorship": {
      "createdLabel": "被创造 由 <%=createdBy%>创建 基于/在 <%=createdDate%>",
      "modifiedLabel": "修改的 由 <%=modifiedBy%>修改 基于/在 <%=modifiedDate%>"
    },

lastly, the authorship.html in this Atlas git will be changed like:

<div data-bind="css: classes('container')">
    <span data-bind="visible: createdBy, 
        text: ko.i18nformat('components.authorship.createdLabel', 
                    'by <%=createdBy%> on <%=cratedDate%>', 
                    {createdBy: createdBy, createdDate: createdDate})"></span><span data-bind="visible: modifiedDate">,</span>
    <span data-bind="visible: modifiedDate, 
        text: ko.i18nformat('components.authorship.modifiedLabel', 
                    'by <%=modifiedBy%> on <%=modifiedDate%>', 
                    {modifiedBy: modifiedBy, modifiedDate: modifiedDate})"></span>
</div>

If the codes above are the same as you think, I will commit the codes again.

[chrisknoll]

Yes, that was exactly what I was thinking. Reduces i18n elements and simplifies code.

I think we have to handle the 'version created' case, which is displayed when you are looking at a version of an asset. It should follow the same pattern to say Version created on <%=createdDate%>, but I'd have to look at the code to identify where that i18n element is used to see how they are building the label.

[tandara0]

I tried adding preview element and removing versionCreated in the authorship component. And in html, I added previewText. Instead, the createdText is no more used. The previewText will be set in the getAuthorship() javascript function.

messages_en.json: preview is added and previewText is used. remove versionCreated.

    "authorship": {
      "preview": "version ",
      "createdLabel": "<%=previewText%>created by <%=createdBy%> on <%=createdDate%>",
      "modifiedLabel": "modified by <%=modifiedBy%> on <%=modifiedDate%>"
    },

cohort-definition-manager.js : previewText is added.

  getAuthorship() {
    const cohortDef = this.currentCohortDefinition();

    let createdText, createdBy, createdDate, modifiedBy, modifiedDate, previewText;

    if (this.previewVersion()) {
        previewText = ko.i18n('components.authorship.preview', 'version ');
                 ......
    } else {
        previewText = null;
                 .......
    }

    return {
        createdText: createdText,
        createdBy: createdBy,
        createdDate: createdDate,
        modifiedBy: modifiedBy,
        modifiedDate: modifiedDate,
        previewText: previewText
    }
  }

Like above code changes of cohort-defined-manager.js, all javascript with getAuthorship() function defined should be changed.

authorship.js: previewText is added.

    class Authorship extends AutoBind(Component) {
        constructor(params) {
            super(params);
            this.createdText = params.createdText || ko.i18n('components.authorship.created', 'created');
            this.createdBy = params.createdBy;
            this.createdDate = params.createdDate;
            this.modifiedBy = params.modifiedBy;
            this.modifiedDate = params.modifiedDate;
            this.previewText = params.previewText;
        }
    }

authorship.html: use previewText.

<div data-bind="css: classes('container')">
    <span data-bind="visible: createdBy,
        text: ko.i18nformat('components.authorship.createdLabel',
                        '<%=previewText%>created by <%=createdBy%> on <%=cratedDate%>',
                        {createdBy: createdBy, createdDate: createdDate, previewText:previewText})"></span><span data-bind="visible: modifiedDate">,</span>
    <span data-bind="visible: modifiedDate,
        text: ko.i18nformat('components.authorship.modifiedLabel',
                        'modified by <%=modifiedBy%> on <%=modifiedDate%>',
                        {modifiedBy: modifiedBy, modifiedDate: modifiedDate})"></span>
</div>

Finally, the createdText is no more used, and can be eliminated.

[chrisknoll]

Ok, thanks for explaining this, and seems fine. Only thing I would change is on the naming: the 'preview' is probably better termed a 'prefix' or 'preamble', prefix is probably fine enough (as it represents something inserted before something else). while preamble is more like a introduction to a story. So I'd suggest prefix over preview (preview has different meaning in the application where you can 'preview' a concept set change before commit).

[tandara0]

Although I don't want to insist on my codes, I think I have to add a explanation as there is possible that you misunderstood something.

I will show you the changed json of messages_ko.json

messages_ko.json:

    "authorship": {
      "preview": "버전 ",
      "createdLabel": "<%=createdBy%>가 <%=createdDate%> 경에 <%=previewText%>생성",
      "modifiedLabel": "<%=modifiedBy%>가 <%=modifiedDate%> 경에 수정"
    },

As you can see in this json, the previewText is not positioned in front of the sentense, in korean. (It is not used as prefix.) I named preview because the components.authorship.versionCreated is used only when the contents is in previewVersion. (see getAuthorship() in cohort-definition-manager.js.)

I agree the name 'preview' is not the best. Other candidates are likely to be 'version', 'previewVersion', 'versionCreated', or else.

So, just think about it one more time and make a decision, then I'll follow any decision.

[chrisknoll]

Ok, thanks for the clarification. So I think in order to to deal with different sentence structure across languages, we need to think in terms of labels. I think i understand that you are saying that the 'preview' element positioning depends on langauge. So i think we need to think about making it a complete sentence (for the preview label) so it handles the differences.

I don't use the version history so much that I don't know if it even matters that we have to have special messaging about authorship for a preview vs. the normal, non-version-history view. I would think that when the version history shows up, you have something in the dialog box or in the header that says 'this is a version'. So could we remove the preview text from the authorshp component completely?

[tandara0]

Sure. I remove the preview text. Then I committed both Atlas and WebAPI.

The WebAPI PR: https://github.com/OHDSI/WebAPI/pull/2382

Please, review them.