Skip to content

JENKINS-62285 Don't use browser alert for notifications and confirmation #60

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.

Sign up for GitHub

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

Closed
aytuncbeken wants to merge 3 commits into from
Closed

JENKINS-62285 Don't use browser alert for notifications and confirmation #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
39516b9
JENKINS-62285
May 27, 2020
7239149
additional location.reload() removed
May 27, 2020
5f3f128
additional location.reload() removed
May 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ...urces/io/jenkins/plugins/folderauth/FolderAuthorizationStrategyManagementLink/index.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
<l:layout permission="${app.ADMINISTER}" norefresh="true" title="${%title}">
<st:include it="${app}" page="sidepanel.jelly" optional="true"/>
<l:main-panel>
<script src="${rootURL}/plugin/folder-auth/js/notification.js"/>
<script src="${rootURL}/plugin/folder-auth/js/addrole.js"/>
<script src="${rootURL}/plugin/folder-auth/js/folders.js"/>
<script src="${rootURL}/plugin/folder-auth/js/managesids.js"/>
Expand Down
21 changes: 10 additions & 11 deletions src/main/webapp/js/addrole.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
const addGlobalRole = () => {
const roleName = document.getElementById('globalRoleName').value;
if (!roleName || roleName.length < 3) {
alert('Please enter a valid name for the role to be added.');
showNotificationERROR('Please enter a valid name for the role to be added.');
return;
}

Expand All @@ -17,7 +17,7 @@ const addGlobalRole = () => {
};

if (response.permissions.length <= 0) {
alert('Please select at least one permission');
showNotificationERROR('Please select at least one permission');
return;
}

Expand All @@ -31,7 +31,7 @@ const addGlobalRole = () => {
const addFolderRole = () => {
const roleName = document.getElementById('folderRoleName').value;
if (!roleName || roleName.length < 3) {
alert('Please enter a valid name for the role to be added');
showNotificationERROR('Please enter a valid name for the role to be added');
return;
}

Expand All @@ -42,12 +42,12 @@ const addFolderRole = () => {
};

if (!response.permissions || response.permissions.length <= 0) {
alert('Please select at least one permission');
showNotificationERROR('Please select at least one permission');
return;
}

if (!response.folderNames || response.folderNames.length <= 0) {
alert('Please select at least one folder on which this role will be applicable');
showNotificationERROR('Please select at least one folder on which this role will be applicable');
return;
}

Expand All @@ -61,7 +61,7 @@ const addFolderRole = () => {
const addAgentRole = () => {
const roleName = document.getElementById('agentRoleName').value;
if (!roleName || roleName.length < 3) {
alert('Please enter a valid name for the role to be added');
showNotificationERROR('Please enter a valid name for the role to be added');
return;
}

Expand All @@ -72,12 +72,12 @@ const addAgentRole = () => {
};

if (!response.permissions || response.permissions.length <= 0) {
alert('Please select at least one permission');
showNotificationERROR('Please select at least one permission');
return;
}

if (!response.agentNames || response.agentNames.length <= 0) {
alert('Please select at least one agent on which this role will be applicable');
showNotificationERROR('Please select at least one agent on which this role will be applicable');
return;
}

Expand All @@ -100,10 +100,9 @@ const sendPostRequest = (postUrl, json) => {

xhr.onload = () => {
if (xhr.status === 200) {
alert('The role was added successfully');
location.reload(); // refresh the page
showNotificationOK('The role was added successfully');
} else {
alert('Unable to add the role\n' + xhr.responseText);
showNotificationERROR('Unable to add the role\n' + xhr.responseText);
}
};

Expand Down
2 changes: 1 addition & 1 deletion src/main/webapp/js/folders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const getFolders = () => {
};

xhr.onerror = () => {
alert(`Unable to get the list of folders: ${xhr.responseText}`);
showNotificationERROR(`Unable to get the list of folders: ${xhr.responseText}`);
};
};

Expand Down
14 changes: 6 additions & 8 deletions src/main/webapp/js/managesids.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,15 @@ function assignSid(roleType, roleName, sidInputBoxId) {
request.open('POST', url);
request.onload = () => {
if (request.status === 200) {
alert('Sid added successfully.');
location.reload();
showNotificationOK('Sid added successfully.');
} else {
alert('Unable to remove the sid.' + request.responseText);
showNotificationERROR('Unable to remove the sid.' + request.responseText);
}

};

request.onerror = () => {
alert('Unable to add the sid to the role: ' + request.responseText);
showNotificationERROR('Unable to add the sid to the role: ' + request.responseText);
};

// see addRole.js
Expand Down Expand Up @@ -59,15 +58,14 @@ function removeSid(roleType, roleName, sidInputBoxId) {
request.open('POST', url);
request.onload = () => {
if (request.status === 200) {
alert('Sid removed successfully.');
location.reload();
showNotificationOK('Sid removed successfully.');
} else {
alert('Unable to remove the sid.' + request.responseText);
showNotificationERROR('Unable to remove the sid.' + request.responseText);
}
};

request.onerror = () => {
alert('Unable to remove the sid from the role: ' + request.responseText);
showNotificationERROR('Unable to remove the sid from the role: ' + request.responseText);
};

// see addRole.js
Expand Down
16 changes: 16 additions & 0 deletions src/main/webapp/js/notification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
function showNotification(message, options, reload = false) {
notificationBar.show(message, options);
if (reload) {
window.setTimeout(function () {
location.reload();
}, notificationBar.DELAY);
}
}

function showNotificationOK(message) {
showNotification(message, notificationBar.OK, true);
Copy link
Member

@AbhyudayaSharma AbhyudayaSharma Jun 8, 2020

Choose a reason for hiding this comment

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

There's a small problem with this behaviour. The page only refreshes when the success notification disappears. So for about 2 seconds, the notification shows but the content is not updated. With the alerts, as soon as the user clicked OK, the page was refreshed. Would it be possible to use a modal popup here? Also note that I'm working on #58 which will completely change the UI.

Copy link
Author

@aytuncbeken aytuncbeken Jun 9, 2020

Choose a reason for hiding this comment

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

Hi, thanks for your comment. I will search for modal solution, not sure though if its possible or not. For providing similar functionality I added optional reload feature, which waits for notification.DELAY.

About the UI change, of course It is up to you. We can apply this change until the new UI is published.

@timja Do you know any examples for Modal implementation ? Thanks.

Copy link
Member

@timja timja Jun 9, 2020

Choose a reason for hiding this comment

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

better to not refresh the page and to do a live update, or let the user refresh it themselves really.

Copy link
Author

@aytuncbeken aytuncbeken Jun 9, 2020

Choose a reason for hiding this comment

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

I think that is topic of the UI change that @AbhyudayaSharma mentioned. Should we decline this request then ?

Copy link
Member

@timja timja Jun 9, 2020

Choose a reason for hiding this comment

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

Yeah, the code will likely still be useful, thanks for doing this!

Copy link
Author

@aytuncbeken aytuncbeken Jun 9, 2020

Choose a reason for hiding this comment

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

No problem

}

function showNotificationERROR(message) {
showNotification(message, notificationBar.ERROR);
}