-
Notifications
You must be signed in to change notification settings - Fork 208
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
Add option to restart jobs upon comment submission #5971
base: master
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
7d7f709
to
e886fd5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5971 +/- ##
=======================================
Coverage 98.98% 98.98%
=======================================
Files 395 395
Lines 39354 39364 +10
=======================================
+ Hits 38954 38964 +10
Misses 400 400 ☔ View full report in Codecov by Sentry. |
e886fd5
to
b1590ed
Compare
This comment was marked as resolved.
This comment was marked as resolved.
The difference between this PR and the previews is that the old PR solves the problem integrating the logic in the Comments.pm passing parameters in the underline function. |
You could base your PR on #5981 but it looks like I'll have to put a little bit more work into it (as CI checks are currently failing). |
dd44e25
to
e188f9e
Compare
7577c7e
to
c07da9d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
c07da9d
to
d98507f
Compare
d98507f
to
f33859a
Compare
Some comments on 717e152
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
A general request: Could you rebase your PRs more often on master? |
717e152
to
1a241aa
Compare
1a241aa
to
d2d169f
Compare
assets/javascripts/openqa.js
Outdated
if (singleJobId) { | ||
newJobUrl = responseJSON.test_url[0][singleJobId]; | ||
} else { | ||
var testUrlData = responseJSON.test_url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var testUrlData = responseJSON.test_url; | |
const testUrlData = responseJSON?.test_url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@b10n1k You missed the ?
here
setTimeout(() => { | ||
window.location.reload(); | ||
}, 2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setTimeout(() => { | |
window.location.reload(); | |
}, 2000); | |
setTimeout(() => window.location.reload(), 2000); |
Although I don't like this whole approach of first adding an info to only reload the whole page 2 seconds later anyway. I would rather not reload the page automatically and change the info to "Changes have been applied. You may reload the page." where "reload" is a link that will reload the page. This way the info contains more useful information and one has more than 2 seconds to read it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of having time to read that a reload is required? Why not just reload right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to the manual reload with a similar text as it is for comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that this has been updated. As-is it looks like you still can't decide whether to reload the page or show a notification.
I suggest to show a notification. Only this way it is apparent that the operation was actually successful. Otherwise the page is just reloading but the user has to interpret this behavior as success - which is not ideal in my opinion. The notification should also explicitly list of jobs that have actually been successfully restarted so one can easily go though the list to access them if wanted. It can of course also include a link for reloading the page.
trigger-edit-button" onclick="showAddCommentsDialog()" title="Restart or comment jobs"> | ||
<span class="fa-stack" style="font-size: 0.9em" > | ||
<i class="fa fa-comment fa-stack-2x text-danger-info" aria-hidden="true"></i> | ||
<i class="fa fa-undo fa-stack-1x" style="color: #0c4374;" aria-hidden="true"></i> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should add a CSS class for this color instead of defining it in HTML. Maybe we can then also use some existing CSS color values defined by Bootstrap or in our SCSS theme. You should also check whether this looks good in darkmode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked in the dark mode and looks good.
styles moved to overview.scss
subtest "job template names displayed on 'Test result overview' page" => sub { | ||
$driver->get('/group_overview/1002'); | ||
is($driver->find_element('.progress-bar-unfinished')->get_text(), | ||
'1 unfinished', 'The number of unfinished jobs is right'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We normally start test names with lower case. You can also drop the "is right" in the end which is kind of a given for checks:
'1 unfinished', 'The number of unfinished jobs is right'); | |
'1 unfinished', 'expected number of unfinished jobs'); |
EDIT: I now see that you're just moving this code. My suggestion would nevertheless be a good improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@b10n1k How about this one?
- Renamed `addComments` to `restartOrCommentJobs` to handle both commenting and restarting jobs. - Added button-specific actions using `form.clickedButton`. - Updated `fetchWithCSRF` logic to dynamically determine request URLs based on the button clicked. - Introduced separate success and error messages for restarting and commenting actions. - Updated HTML to include "Restart and Comment" and "Comment" buttons with individual API endpoints via formaction. That removes action attribute from the form element. - Let "comments" be `btn-warning` and "restart" show more severe operation using `btn-danger`. - Adjusted modal to allow submission of comments or restarts through the correct action handlers. - Inform overview page with warning in case of errors in Response. - Redesign `restartOrCommentJobs` (previews addComments) to reflect the new features. These updates enhance the user experience by allowing users to either comment on jobs, or perform restart with a comment simultaneously from the overview page. Comments always add to the original job of the overview page. By integrating this functionality we provide a workflow for users managing multiple jobs on the overview page using the already existing API implementations. https://progress.opensuse.org/issues/166559 Signed-off-by: Ioannis Bonatakis <[email protected]>
Refactor restartJob in order to make sure that is able to handle request for multiple restarts. This follows up with some changes to the form and the javascript part of the overview.html.ep, which triggers the restart with the use of restartJob, as opposed of reimplement the logic. The advantage is that we have one common behavior and error handling on the restart operation. - An optional comment can be passed not to restartJob, which is included to the request. - restartJob handles both single and multible job restarts. - The form is designed to handle each occasion in separate functions making the code becomes more modular and easier to maintain. - The overvies's restartJobs function will restart only jobs with valid id. https://progress.opensuse.org/issues/166559 Signed-off-by: Ioannis Bonatakis <[email protected]>
d2d169f
to
6a99188
Compare
Great PR! Please pay attention to the following items before merging: Files matching
This is an automatically generated QA checklist based on modified files. |
setTimeout(() => { | ||
window.location.reload(); | ||
}, 2000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see that this has been updated. As-is it looks like you still can't decide whether to reload the page or show a notification.
I suggest to show a notification. Only this way it is apparent that the operation was actually successful. Otherwise the page is just reloading but the user has to interpret this behavior as success - which is not ideal in my opinion. The notification should also explicitly list of jobs that have actually been successfully restarted so one can easily go though the list to access them if wanted. It can of course also include a link for reloading the page.
|
||
.group-comment-icon-stack .fa-undo { | ||
color: #0c4374; /* Your specified color */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment makes no sense. I suggest you remove it.
$driver->get('/tests/overview?state=done&result=failed'); | ||
$driver->find_element('button[title="Restart or comment jobs"]')->click; | ||
my $comment_text = 'comment current jobs and restart'; | ||
my $submit_button = $driver->find_element('#add-comments-controls button[id="restartAndCommentJobsBtn"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This selector is needlessly complicated. If the button has an ID use it directly. There cannot be another button under a different element with the same ID on that page.
my $submit_button = $driver->find_element('#add-comments-controls button[id="restartAndCommentJobsBtn"]'); | |
my $submit_button = $driver->find_element('#restartAndCommentJobsBtn'); |
<button type="submit" class="btn btn-danger"> | ||
<i class="fa fa-comment"></i> Submit comment on all <%= scalar @$job_ids %> jobs | ||
<button type="button" class="btn btn-warning" | ||
value="commentJobs" id="commentJobsBtn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value seems unused so I suggest you remove it.
</button> | ||
<button type="button" class="btn btn-secondary" data-bs-dismiss="modal">Discard</button> | ||
<button type="button" class="btn btn-danger" | ||
value="restartAndCommentJobs" id="restartAndCommentJobsBtn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value seems unused so I suggest you remove it.
|
||
<div class="flex-fill"><i class="fa fa-cog fa-spin | ||
fa-fw"></i> Saving…</div> | ||
<div class="progress" id="restart-progress-bar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ID is not used. But is that right? Wouldn't one want to set the progress bar value at some point? It looks like this code is actually just a leftover. I suggest to stick with a simpler spinning loading indication anyway.
<div class="modal-header"> | ||
<h4 class="modal-title">Add comment on all currently shown jobs</h4> | ||
<button type="button" class="btn-close" data-bs-dismiss="modal"></button> | ||
</div> | ||
<div class="modal-body"> | ||
%= include 'comments/add_comment_form_groups', group_comment => 0, nosubmit => 1 | ||
</div> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class="flex-fill"><i class="fa fa-cog fa-spin fa-fw"></i> Saving…</div> | ||
|
||
<div class="flex-fill"><i class="fa fa-cog fa-spin | ||
fa-fw"></i> Saving…</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you break/wrap this line? It looked much nicer as a single line and without the blank line before it.
<button type="button" class="btn btn-secondary btn-circle btn-sm | ||
trigger-edit-button" onclick="showAddCommentsDialog()" title="Restart or comment jobs"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't wrap this line in the middle of an attribute value.
<i class="fa fa-comment" aria-hidden="true"></i> | ||
<button type="button" class="btn btn-secondary btn-circle btn-sm | ||
trigger-edit-button" onclick="showAddCommentsDialog()" title="Restart or comment jobs"> | ||
<span class="fa-stack group-comment-icon-stack" > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<span class="fa-stack group-comment-icon-stack" > | |
<span class="fa-stack group-comment-icon-stack"> |
addComments
tobulkJobsAction
to handle both commenting and restarting jobs.form.clickedButton
.fetchWithCSRF
logic to dynamically determine request URLs based on the button clicked.These updates enhance the user experience by allowing users to either comment on jobs, or perform restart with a comment simultaneously from the overview page. Comments always add to the original job of the overview page. By integrating this functionality we provide a workflow for users managing multiple jobs on the overview page.
https://progress.opensuse.org/issues/166559