Skip to content

Conversation

@eduardomozart
Copy link
Contributor

@eduardomozart eduardomozart commented Jan 22, 2025

The plug-in stores the error message based on the current user language. This PR stores the message on English and translates the message on the client side, so on GLPI instances with multilanguage user, the task log will show the error message based on the current user language instead of the language of the user that the error message has been generated.

Before this PR, an Portuguese language user was seeing error messages on English:

image

After this PR, a Portuguese Language user can see error messages in their native language independent of the language of the user that the error has been generated:

image

Edit: The only known issue I can think of this PR can cause is that those strings would be removed from the POT file, so I believe it would need a way to keep them there or do a dummy call/register of them into JS/PHP code (maybe a fake/unused property of the same object that I modified into this PR) so they would be gathered by Transiflex bot and wouldn't be missing.

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • This change requires a documentation update.

Description

  • It fixes # (issue number, if applicable)
  • Here is a brief description of what this PR does

Screenshots (if appropriate):

The plug-in stores the error message based on the current user language. This PR stores the message on English and translates the message on the client side, so on GLPI instances with multilanguage user, the task log will show the error message based on the current user language instead of the language of the user that the error message has been generated.
Copy link
Collaborator

@trasher trasher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, strings will never be translated at all, because sources won't be extracted to POT file.

@stonebuzz
Copy link
Collaborator

stonebuzz commented Jan 23, 2025

@trasher I think so, thanks to this

image

Now the string is translated on demand (depending on the user's language)

@trasher
Copy link
Collaborator

trasher commented Jan 23, 2025

@stonebuzz nope. Strings have to exist in source code, and be "marked" as translatable for extraction to work.

So this code cans indeed give a translated value, my comment concerns source strings extraction from source code.

@stonebuzz
Copy link
Collaborator

@stonebuzz nope. Strings have to exist in source code, and be "marked" as translatable for extraction to work.

So this code cans indeed give a translated value, my comment concerns source strings extraction from source code.

I see, you're right

@stonebuzz
Copy link
Collaborator

We should keep the call to gettext (__(‘string to translate’, ‘context’) so that the CI can extract the string and translate it in Transifex.

ex :

if (
    $result['run']['state'] == PluginGlpiinventoryTaskjobstate::SERVER_HAS_SENT_DATA
        or $result['run']['state'] == PluginGlpiinventoryTaskjobstate::AGENT_HAS_SENT_DATA
) {
    $msg = "The agent is requesting a configuration that has already been sent to him by the server. It is more likely that the agent is subject to a critical error.";
    $translatable_msg = __($msg, 'glpiinventory');

    $jobstates_to_cancel[$jobstate->fields['id']] = [
        'jobstate' => $jobstate,
        'reason'   => $msg
        'code'     => $jobstate::IN_ERROR
    ];
    continue;
}

@trasher
Copy link
Collaborator

trasher commented Jan 23, 2025

We should keep the call to gettext (__(‘string to translate’, ‘context’) so that the CI can extract the string and translate it in Transifex.

ex :

if (
    $result['run']['state'] == PluginGlpiinventoryTaskjobstate::SERVER_HAS_SENT_DATA
        or $result['run']['state'] == PluginGlpiinventoryTaskjobstate::AGENT_HAS_SENT_DATA
) {
    $msg = "The agent is requesting a configuration that has already been sent to him by the server. It is more likely that the agent is subject to a critical error.";
    $translatable_msg = __($msg, 'glpiinventory');

    $jobstates_to_cancel[$jobstate->fields['id']] = [
        'jobstate' => $jobstate,
        'reason'   => $msg
        'code'     => $jobstate::IN_ERROR
    ];
    continue;
}

Nope, this is still wrong :/

String to translate must be in gettext function (gettext extraction will not execute any php code):

    $msg = "The agent is requesting a configuration that has already been sent to him by the server. It is more likely that the agent is subject to a critical error.";
    $translatable_msg = __("The agent is requesting a configuration that has already been sent to him by the server. It is more likely that the agent is subject to a critical error.", 'glpiinventory');

@eduardomozart
Copy link
Contributor Author

I updated this PR following the instructions from @stonebuzz and @trasher.

trasher
trasher previously approved these changes Jan 24, 2025
@trasher trasher requested a review from stonebuzz January 24, 2025 07:23
@trasher
Copy link
Collaborator

trasher commented Jan 24, 2025

Tests are failing... :/
It's surprising; maybe test itself is incorrect - I do not have time to check.

@eduardomozart eduardomozart mentioned this pull request Jan 24, 2025
3 tasks
@eduardomozart
Copy link
Contributor Author

Hello @trasher, now the tests works as expected.

@trasher
Copy link
Collaborator

trasher commented Jan 24, 2025

I do not understand the error, and I do not understand the fix neither. Nor why this is breaking now - seems unrelated.

As far a we can see, it can have several causes; I'd prefer to be sure understanding the fail.

@trasher trasher dismissed their stale review January 24, 2025 14:02

Need to understand tests failure

@eduardomozart
Copy link
Contributor Author

eduardomozart commented Jan 24, 2025

From what I've seem while testing at #606, it seems that the task ID wasn't being selected correctly and the task status wasn't being updated to success at https://github.com/eduardomozart/glpi-inventory-plugin/blob/a9bd8766d1282804829ccb4747878bbe5d18ee0d/tests/Integration/Tasks/CronTaskTest.php#L701, which also makes the test at https://github.com/eduardomozart/glpi-inventory-plugin/blob/a9bd8766d1282804829ccb4747878bbe5d18ee0d/tests/Integration/Tasks/CronTaskTest.php#L753 fails because the task status wasn't being set as completed.
Edit: but even without this fix I was able to pass test on other PRs, so I don't think it's the case. I saw that @stonebuzz created a PR #607 to troubleshoot this further, too.

@stonebuzz
Copy link
Collaborator

Oo => #607

no error XD

@stonebuzz
Copy link
Collaborator

@eduardomozart

can you revert last commit about test ? to try

@eduardomozart
Copy link
Contributor Author

eduardomozart commented Jan 24, 2025

I believe there should be an error on my branch, I'm unable to commit on it using GitHub Desktop, only through GitHub WebUI. As iCloud sync is enabled for Documents folder, I think that while changing branches it get lost. I'll try to revert my last commit.

@stonebuzz
Copy link
Collaborator

😄

maybe a problem with Github?
I doubt the problem is with your branch

@trasher trasher merged commit 1764dca into glpi-project:main Jan 24, 2025
8 checks passed
@eduardomozart eduardomozart mentioned this pull request Jan 27, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants