-
Notifications
You must be signed in to change notification settings - Fork 175
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
[issue_tracker] Add Batch Mode #9339
Conversation
Great start Ayush! Could you please embed here (or else link) the video, or screenshots are fine. For the missing modal - could this be fixed before this work is presented to the larger team at the end of September, to avoid confusion and so Debug mode can be fully useful --: The reason why --: @racostas @regisoc let me know asap if this has already been discussed among you. Thanks Ayush, it's looking solid otherwise. |
Hi @ay-bh, I reviewed the PR and is looking like a good start indeed. |
@SantiagoTG, your inputs here are also very welcome since you have been working a loot with issue tracker. If you have time to take a look to this proposal you opinion is very important. Thanks. |
@racostas @ay-bh to keep you updated, we did not have time to test that together with @SantiagoTG. |
Hello @ay-bh great start indeed! Here are some comments from both me and @SantiagoTG. I tried to group comments:
That was a lot more to write than expected, but I tried to describe each point. |
Hi @regisoc, very good review in facts. I'm sure @ay-bh can address all of this points before the end of the coding period (end of October). Maybe he can present some of the advances he had been making in two weeks from now. (then do a second final presentation in November 5). I will keep you posted with more details after consulting it with him because he have other commitments in the school as well. |
Can we also make sure this has a more descriptive name than "Debug View"? I suggest something like "Batch Mode" since that seems to be what it's doing. |
Thank you all for your detailed feedback and suggestions. I'll start working on implementing these improvements. |
Hi @ay-bh, this two changes are looking good. Thanks for the update ! Great work ! |
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.
To add with @racostas previous review:
- pagination change at the bottom.
- pagination starts at 10 (yes!).
- link to all comments. Could be nice, but I think this one is less a priority. All comments can be accessed by clicking the issue ID/title for the full issue view.
Points to be addressed (previous review):
- issues pagination: looks ok, but do not have enough issues to test. Maybe easier to test with a pagination at 10 ;)
- issues ordering. Not a priority.
- bug: issue timestamp not updating: resolved.
- display last comments: looks nice!
- clearer disabled elements: if possible, change dropdown to a label when it is disabled + add the actual label (category,priority,status). i.e. seomthing like
category: aaa
. - new modal for comments: partially done. Still needs to modify assigned/watchers but it looks simple and great. Tested with 5 paragraphs of lorem ipsum and still looks nice!
- filter block: more genral comment here. The actual design is better but we still cannot track how many filters are selected. I was more thinking about something like this where: we can direct access to all filter in one click, have a visual hint on which filters have selected criteria (badge/number), and collapse the full filter section to focus on issues.
- bug: issue with description text area: ok.
- filter reset button.
- Site filters.
Hi @ay-bh, very nice. The rest very nice ! |
Thank you for reviewing, @racostas. Since @regisoc didn't mention it, and I felt it is not something that is updated regularly, I decided not to include it there to keep the UI clean. Instead, I added it just below the assignee on the top-right. |
@regisoc, according to your experience the site is something the change frequently ? I kind of agree with @ay-bh in this point but I want your inputs since you deal more with users than myself. I other fronts, the rest of the PR looks good to me. All core functionalities and issues were addressed (in my opinion ). I thinks this PR is ready to be merged. The functionalities that were not high priority could be part of a second PR since this one is getting already big, what do you think @regisoc , @christinerogers ? |
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.
LGTM.
I reviewed and seems to be working fine. I think other functionalities could be addressed in a second PR.
Approved by my end !
Great work @ay-bh in your Demo today and discussion with the whole LORIS team, this feature is well done and greatly appreciated. Notes from today: nice to haves and issues for follow-up:
Otherwise, no current blockers to getting this merged. |
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.
Hello all @ay-bh @racostas @christinerogers, I am happy with this result.
As I said in LORIS meeting, it is a really strong start for a feature that will be useful and is already waited in some cases/projects.
@racostas in my experience, no, site are not changing frequently, not an issue immediately, can be done later on. Already told Ayush that is the case.
I added comments. To sum up: 1/ finish the missing parts, then 2/ stress test this feature. The main goal of this feature was to have a new front-end i.e. an alternative view, but I suspect it can be long to load with lots of issues. Waiting elements to load is one of the main source of frustration.
From what I can see, points for next PR, ordered by priority:
assignee/watchers changes
in comment modal.permission
to access this feature, as I do not see it required nor useful for all users (can be discussed).- Change the
switch mode
button to another, more convenient location. Maybe close to new issue (button only appearing with the right permission).
site change
in card main panel.
Points later on (because not part of the original ticket/issue, but still important):
- stress test i.e. push 1k, 5k, 10k, 20k, 50k issues with various number of comments and see how the feature behaves.
- order card elements in main panel.
- batch-close multiple issues at once.
Still, big thanks @ay-bh for this PR, great work!
|
||
if ($batch_mode) { | ||
// Fetch all issues | ||
$allIssues = $this->_getAllIssues($user); |
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.
Do we reload all issues every time the batch mode is selected? Maybe something to memoize for both sides or use a lazy loading strategy.
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.
It's a different endpoint with different fields, for e.g. top 3 comments, So memoizing might not make sense. Also the user may not toggle between the modes frequently.
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.
You might be right on the fact they will not change frequently.
I just want to make sure it does not affect too much the wait time.
Good for now.
import PropTypes from 'prop-types'; | ||
import swal from 'sweetalert2'; | ||
import Modal from 'jsx/Modal'; | ||
import '../css/issue_card.css'; |
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.
Normally we inject css from php, but no php backend specific to these new elements, looks ok.
IssueCard.propTypes = { | ||
issue: PropTypes.shape({ | ||
issueID: PropTypes.number.isRequired, | ||
title: PropTypes.string.isRequired, | ||
reporter: PropTypes.string.isRequired, | ||
assignee: PropTypes.string, | ||
status: PropTypes.string.isRequired, | ||
priority: PropTypes.string.isRequired, | ||
module: PropTypes.number, | ||
dateCreated: PropTypes.string.isRequired, | ||
lastUpdate: PropTypes.string, | ||
lastUpdatedBy: PropTypes.string, | ||
sessionID: PropTypes.number, | ||
centerID: PropTypes.number, | ||
candID: PropTypes.number, | ||
category: PropTypes.string, | ||
instrument: PropTypes.string, | ||
description: PropTypes.string, | ||
PSCID: PropTypes.string, | ||
visitLabel: PropTypes.string, | ||
topComments: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
issueComment: PropTypes.string.isRequired, | ||
dateAdded: PropTypes.string.isRequired, | ||
addedBy: PropTypes.string.isRequired, | ||
}) | ||
).isRequired, | ||
}).isRequired, | ||
onUpdate: PropTypes.func.isRequired, | ||
statuses: PropTypes.object.isRequired, | ||
priorities: PropTypes.object.isRequired, | ||
categories: PropTypes.object.isRequired, | ||
sites: PropTypes.object.isRequired, | ||
}; |
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.
That is a chunky element!
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.
Yeah, it's actually all the data that's coming in issuecard. I wrote it this way so it's clear what all we have. If it's too big, I could maybe just write issue: PropTypes.something
, but this way someone else working on this in future may not know what all we are receiving.
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 issues, that is fine. Having all in one place is actually very representative of the central element.
I just did not realize it was that big :)
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 a picky question: why using this file (i.e. edit.class.inc
) instead of issue.class.inc
for retrieving all issues? Not important, just curious.
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 get issue logic for an individual issue (when editing the issue through traditional view) is in the same file.
Hi @driusan. This one it's ready for final review. If you have time to final review this one it will be great since he have less than 2 weeks now to wrap up all his work (including a PR for the "not priority" issues) Thank you @driusan and sorry for this been need with a bit of pressure from our side. |
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.
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 have reservations about the use of fragments and the way the getAllIssues query is a hardcoded database query instead of using the issues provisioner but not enough to block the PR
Add a new "Batch mode" view to the issue tracker which allows users to modify many issues in bulk, rather than having to go into each individual issue to modify them. * Resolves aces#9268
Brief summary of changes
"Debug View""Batch Mode" in Issue Tracker moduleThe following features will be implemented in a future PR:
issue_tracker_debug_mode_edit
Testing instructions (if applicable)
Link(s) to related issue(s)
Demo
Demo.mov