-
Notifications
You must be signed in to change notification settings - Fork 4
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
Instrument Scheduler #998
Instrument Scheduler #998
Conversation
resources/schemas/dbscripts/postgresql/targetedms-24.005-24.006.sql
Outdated
Show resolved
Hide resolved
resources/schemas/dbscripts/postgresql/targetedms-24.005-24.006.sql
Outdated
Show resolved
Hide resolved
projectDropDown += '<select id="projectDropDown" name="projectDropDown">'; | ||
for (let i = 0; i < rows.length; i++) { | ||
// if project == rows[i].Id, then select this option | ||
projectDropDown += '<option value="' + rows[i].Id + '" ' + (project == rows[i].Id ? 'selected' : '') + '>' + LABKEY.Utils.encodeHtml(rows[i].title) + '</option>'; |
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 know Id should be a number but safest to encode it too since JavaScript is untyped
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.
done
var content = ''; | ||
|
||
for(var i in events) { | ||
// format 12/01/24, 10:00 AM - 12/01/24, 11:00 AM |
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.
Let's respect the server's configured date/time formats instead of hard-coding here. See LABKEY.container.formats
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.
changed
'<label for="paymentMethodDropDown">Payment Method: </label>'; | ||
paymentMethodDropDown += '<select id="paymentMethodDropDown" name="paymentMethodDropDown">'; | ||
for (let i = 0; i < rows.length; i++) { | ||
paymentMethodDropDown += '<option value="' + rows[i].Id + '">' + rows[i].name + '</option>'; |
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.
HTML encode
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.
done
let id = 'paymentMethodDropDown' + paymentMethodCount; | ||
let paymentMethodDropDown = '<label for=' + id + '>Payment Method:</label>'; | ||
paymentMethodDropDown += '<select id=' + id + ' name=' + id + '">'; | ||
for (let i = 0; i < paymentMethodsData.length; i++) { | ||
paymentMethodDropDown += '<option value="' + paymentMethodsData[i].Id + '">' + paymentMethodsData[i].name + '</option>'; | ||
} | ||
paymentMethodDropDown += '</select>'; | ||
|
||
paymentMethodDropDown += ' <input type="text" id="paymentMethodPercent" name="paymentMethodPercent" value="0" size="3" maxlength="3">'; |
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.
HTML encode
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.
done
@Override | ||
public boolean hasPermission(@NotNull UserPrincipal user, @NotNull Class<? extends Permission> perm) | ||
{ | ||
return (getContainer().hasPermission(user, ReadPermission.class)); |
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.
Won't this let users who only have read access to the container edit these tables?
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.
changed to AdminPermission
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.
A few more comments. Let me know if you have questions or if there are any updates worthy of another review.
@Override | ||
public boolean hasPermission(@NotNull UserPrincipal user, @NotNull Class<? extends Permission> perm) | ||
{ | ||
return (getContainer().hasPermission(user, AdminPermission.class)); |
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 is this override needed at all? This will prevent non-admins from accessing the table.
<div id="instrumentOperatorDropDown"></div> | ||
<br/> | ||
<div id="paymentMethodDropDown"></div> | ||
<a href="javascript:void(0);" id="addPaymentMethod">[Add Payment Method]</a> |
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 think browsers treat this as inline script and it'll be rejected by our stronger CSP. Do you see any browser console errors? You may need to register this dynamically.
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.
changed
<div class="modal-header"> | ||
<h5 class="modal-title">Add Instrument Time</h5> | ||
<button type="button" class="close" data-dismiss="modal" aria-label="Close"> | ||
<span aria-hidden="true">×</span> |
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.
Misplaced &
?
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.
no, this is the 'X' sign from html entities in the window modal.
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.
Ah, OK! I didn't recognize that entity name. I should have looked it up.
function editEvent(event) { | ||
let startDate = event.startDate; | ||
let endDate = event.endDate; | ||
let startDateVal = startDate.getFullYear() + "-" + |
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.
Can this use the configured date/time too?
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.
done
Rationale
This is the first iteration of the instrument scheduler which will let the users schedule time on instrument and associate the usage with projects and budgets for the billing.
Related Pull Requests
Changes