-
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] Enable editing of site, assignee and watchers in Batch Mode #9425
Conversation
44f42e9
to
5741cbc
Compare
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.
Thanks for this PR, comments:
- Some static test did not pass.
- adding/removing watchers looks good and in sync when switching from edit/detail view to batch mode.
- site dropdown, I tried to change to
No Site
but had the following image show up.
This option should not be added to force user to choose one site.
- issue: unassigned state card text. When no assignee or unassigning someone from an issue, there is a blank in the card. This should instead have an explicit "unassigned" tag. Also, on a second thought, it is in line with the previous comment, not allowing to have a ticket unassigned might be a good idea. The goal of issue tracker is to resolve those tickets. Not having someone assigned, they will be forgotten.
- issue maybe? @racostas I think it is not linked to this PR: when updating the assignee, no history about this change in the edit/detailed view. The change is acted on the batch mode card view, and in the edit/detailed view but not in the history section.
5741cbc
to
1bff72c
Compare
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.
Thanks for this update @ay-bh :
-
No site
issue: is ok, option removed. -
Unassigned
Issue : statesAssignee:None
in card. Front-end looks ok. I am scratching my head here as I see it as mandatory as the previous point. In HBCD, we enforced a default assignee based on category so that issues do not fall into limbo. @christinerogers @racostas does that make sense to you that an issue is not assigned? -
no assignee update in history
issue: could/should be referenced in another issue ticket if necessary.
@racostas @christinerogers If the second point is ok to you, then the PR looks ok to me.
Thanks @regisoc for these comments.
Agree this should be another issue, and it's important. I'll add this to the Followup ticket #9418. @ay-bh if you feel keen to address this on your own time, thank you - it's optional since you have completed GSOC.
@regisoc I get why HBCD has customized this module to make assignee and site selection mandatory, and there are valid reasons these were not mandatory in the Loris module design.
Agreed let's proceed with this PR as is. Over to @racostas for review to move this PR forward. Note this no-site design change requested by regis should be documented in the module readme, and this is one of many points to follow up on in #9418 |
@racostas waiting for your review, I will put you as assignee. |
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 all again and manual tested:
- Functionalities looks good.
- Assignee not been updated in history I checked and effectively is a preexisting issue. I agree we should tackle this. Yes, let's create an issue for it ... or we can added it to [issue_tracker] batch mode - follow up #9418 since this look a very good summary ticket for all
issue_tracker
related developments. Do you think this could be a good idea @regisoc ? - Regarding making the assignee mandatory, I don't have a super strong opinion, I see for example in Redmine is not mandatory, but something we can think about it. It could be a new point in [issue_tracker] batch mode - follow up #9418 as well.
In conclusion, I think this PR is good to go. I leave @driusan the final review / approval.
I saw the two point were already added to #9418 :) |
Brief summary of changes
Testing instructions
Link(s) to related issue(s)
Screenshots