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 various uses of the is_admin helper in templates #1879

Merged
merged 3 commits into from
Nov 21, 2018

Conversation

kraih
Copy link
Member

@kraih kraih commented Nov 21, 2018

The is_admin helper returns a Perl boolean, but was used to generate JavaScript booleans directly here. There was one more use of <%= is_admin %> for setupJobTemplates, but that function does not actually use that argument at all, so i opted to remove it.

While the fix was very simple, i'm still trying to find a good place to test this and am learning Selenium in the process. So please consider this pull request a work in progress for now.

Progress issue: https://progress.opensuse.org/issues/44039

…Script that resulted in empty lists, because the helper returns a Perl boolean value instead of a JavaScript boolean
@Martchus Martchus changed the title Fix various uses of the is_admin helper in templates WIP: Fix various uses of the is_admin helper in templates Nov 21, 2018
@Martchus
Copy link
Contributor

Martchus commented Nov 21, 2018

Changed the title since you're saying it is still WIP. But it looks already good. (Beside the fact that the admin table code isn't a very nice place.)

@@ -2,7 +2,7 @@
% title 'Test suites';

% content_for 'ready_function' => begin
populate_admin_table(<%= is_admin %>, true);
populate_admin_table(<%= is_admin ? 'true' : 'false' %>, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

as you put the same code now in 3 places, this sounds like a good helper candidate

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, do you have a preferred naming convention for js generating helpers?

Copy link
Member Author

Choose a reason for hiding this comment

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

Otherwise i suppose is_admin_js or js_is_admin would work. Not sure which i prefer.

@kraih kraih changed the title WIP: Fix various uses of the is_admin helper in templates Fix various uses of the is_admin helper in templates Nov 21, 2018
@kraih
Copy link
Member Author

kraih commented Nov 21, 2018

Ok, this should be ready now. I've added a new helper and test.

@Martchus
Copy link
Contributor

I re-triggered the failing test because it looks unrelated. If I remember correctly, I have already seen this failure before.

#   Failed test 'No tests run for subtest "Test Minion task registration and execution"'
#   at ./t/25-cache-service.t line 447.
<7>[1374] [d] Process 1481 is performing job "36" with task "cache_asset"
Can't call method "execute" on an undefined value at ./t/25-cache-service.t
	line 442 (#1)
    (F) You used the syntax of a method call, but the slot filled by the
    object reference or package name contains an undefined value.  Something
    like this will reproduce the error:

        $BADREF = undef;
        process $BADREF 1,2,3;
        $BADREF->process(1,2,3);

Uncaught exception from user code:
	Can't call method "execute" on an undefined value at ./t/25-cache-service.t line 442.
	Test::Builder::subtest('Test::Builder=HASH(0x5930ff0)', 'Test Minion task registration and execution', 'CODE(0xcb8f0e8)') called at /usr/lib/perl5/vendor_perl/5.18.2/Test/More.pm line 807
	Test::More::subtest('Test Minion task registration and execution', 'CODE(0xcb8f0e8)') called at ./t/25-cache-service.t line 447

@kraih
Copy link
Member Author

kraih commented Nov 21, 2018

Yea, that test failure can't be related to this pull request.

@codecov
Copy link

codecov bot commented Nov 21, 2018

Codecov Report

Merging #1879 into master will increase coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1879      +/-   ##
==========================================
+ Coverage   90.56%   90.68%   +0.11%     
==========================================
  Files         148      148              
  Lines       10250    10249       -1     
==========================================
+ Hits         9283     9294      +11     
+ Misses        967      955      -12
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Plugin/Helpers.pm 93.98% <100%> (-0.04%) ⬇️
lib/OpenQA/Worker/Engines/isotovideo.pm 96.59% <0%> (-0.57%) ⬇️
lib/OpenQA/WebAPI/Controller/Test.pm 96.14% <0%> (+0.02%) ⬆️
lib/OpenQA/Utils.pm 92.08% <0%> (+0.22%) ⬆️
lib/OpenQA/WebAPI/Controller/Step.pm 84.59% <0%> (+0.36%) ⬆️
lib/OpenQA/WebSockets/Server.pm 92.2% <0%> (+0.45%) ⬆️
lib/OpenQA/Worker/Common.pm 83.33% <0%> (+2.17%) ⬆️
lib/OpenQA.pm 71.79% <0%> (+2.56%) ⬆️
lib/OpenQA/Worker/Commands.pm 88.46% <0%> (+2.56%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7be0d19...e6197a5. Read the comment docs.

@coolo coolo merged commit b543647 into os-autoinst:master Nov 21, 2018
coolo pushed a commit that referenced this pull request Nov 21, 2018
commit b543647
Merge: dee7a2c e6197a5
Author:     Stephan Kulow <stephan@kulow.org>
AuthorDate: Wed Nov 21 19:47:05 2018 +0100
Commit:     GitHub <noreply@github.com>
CommitDate: Wed Nov 21 19:47:05 2018 +0100

    Merge pull request #1879 from kraih/is_admin

    Fix various uses of the is_admin helper in templates
@kraih kraih deleted the is_admin branch May 12, 2020 13:18
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