Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: "uninitialized" warning in the user form for professional accounts #6637

Merged
merged 6 commits into from
Apr 19, 2022

Conversation

yuktea
Copy link
Contributor

@yuktea yuktea commented Apr 17, 2022

Description

Implements an additional check to see if an org associated with the user is defined before executing the code block.
This fixes an uninitialized value warning.

The error

[Sun Apr 17 11:00:58 2022] -e: Use of uninitialized value in concatenation (.) or string at /opt/product-opener/cgi/user.pl line 208.

Implements an additional check to see if an org is defined before
executing this code block. This fixes an uninitialized value warning as
well
@yuktea yuktea requested a review from a team as a code owner April 17, 2022 12:09
cgi/user.pl Outdated
}
]
};
if (defined $user_ref->{org}) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So that definitely will solve the warning, but then it will prevent users to create a professional account. When users create an account, $user_ref->{org} is not defined (yet), but we want to display them the checkbox option to say that it is a professional account.

The warning field should probably be removed, and instead, we can change the template to display the warning message, only when needed.

Template is here : /templates/web/pages/user_form/user_form_page.tt.html

This part should be changed:


                    [% IF accepted_organization && section.id == "professional" %]
                        <p>[% field.warning %]</p>
                    [% ELSE %]
                        <label for="[% field.field %]">
                            <input type="checkbox" id="[% field.field %]" name="[% field.field %]" [% IF field.value == 'on' %]checked="checked"[% END %] />
                            [% field.label %]
                        </label>
                    [% END %]

the Lang.pm defines f_lang() and lang_sprintf() functions that can be used directly in the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing! This is certainly a great approach to fix that warning.
I've pushed another commit with an update :)

@github-actions github-actions bot added the Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. label Apr 17, 2022
cgi/user.pl Outdated Show resolved Hide resolved
[% IF accepted_organization && section.id == "professional" %]
<p>[% field.warning %]</p>
[% IF accepted_organization && section.id == "professional" && user_org.defined %]
<p>[% f_lang('this_is_a_pro_account_for_org', {'org' => user_org} ) %]</p>
Copy link
Contributor

@stephanegigandet stephanegigandet Apr 18, 2022

Choose a reason for hiding this comment

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

Hi @yuktea , have you checked if it works? (you can do that by creating an account with the "this is a professional account" checkbox, and then editing the account).
I think it won't work as is, because f_lang() expects a different format than sprintf (%s vs something like ).
You can look at the strings in po/common/common.pot and po/common.en.po
The best solution is indeed to use f_lang(), but then you have to change the string.

e.g. here is an example for another string:

# variable names between { } must not be translated
msgctxt "f_move_data_and_photos_to_main_language"
msgid "Move all data and selected photos in {language} to the main language of the product: {main_language}"
msgstr "Move all data and selected photos in {language} to the main language of the product: {main_language}"

@github-actions github-actions bot added the Translations We use a non-standard version of GetText, lack language variants support translate.openfoodfacts.org label Apr 18, 2022
Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

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

Looks good!

There's a line that I think is not used (and was not used before):

$template_data_ref->{pro_account_org} = sprintf(lang("this_is_a_pro_account_for_org"),"" . $user_ref->{org} . "");

@stephanegigandet
Copy link
Contributor

Could you also remove the old string "this_is_a_pro_account_for_org" from the common.pot file?

Copy link
Contributor

@stephanegigandet stephanegigandet left a comment

Choose a reason for hiding this comment

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

Thank you!

@yuktea
Copy link
Contributor Author

yuktea commented Apr 18, 2022

Looks good!

There's a line that I think is not used (and was not used before):

$template_data_ref->{pro_account_org} = sprintf(lang("this_is_a_pro_account_for_org"),"" . $user_ref->{org} . "");

Yes! was thinking the same. I'll clean this up

@sonarcloud
Copy link

sonarcloud bot commented Apr 18, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@stephanegigandet stephanegigandet merged commit b9d4fce into main Apr 19, 2022
@stephanegigandet stephanegigandet deleted the fix-warning-uninitialized branch April 19, 2022 12:02
@yuktea yuktea changed the title fix: Add check for the definition of $user_ref->{org} fix: Uninitialized value warning Apr 22, 2022
@yuktea yuktea changed the title fix: Uninitialized value warning fix: "uninitialized" warning in the user form for professional accounts Apr 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Template::Toolkit The templating toolkit used by product opener. The starting point for HTML/JS/CSS fixes. Translations We use a non-standard version of GetText, lack language variants support translate.openfoodfacts.org
Projects
Development

Successfully merging this pull request may close these issues.

3 participants