WIP: Add Labels to Milestones #17265
Conversation
techknowlogick
added
type/feature
| for _, label := range labels { | ||
| // Don't add already present labels and invalid labels | ||
| if hasMilestoneLabel(e, milestone.ID, label.ID) || | ||
| (label.RepoID != milestone.RepoID && label.OrgID != milestone.Repo.OwnerID) { |
There was a problem hiding this comment.
I have the feeling that this check should be extracted into its own function as you use it more than once.
And I'm still confused by it.
It could be called i.e. isMilestoneLabelPairValid or something like that.
| if err != nil { | ||
| return err | ||
| } | ||
| defer committer.Close() |
There was a problem hiding this comment.
Shouldn't the ctx be closed as well, or does the committer suffice?
| } | ||
|
|
||
| func (milestone *Milestone) loadLabels(e db.Engine) (err error) { | ||
| if milestone.Labels == nil { |
There was a problem hiding this comment.
Only this line differs between this method and getLabels:
if len(milestone.Labels) > 0 {
Which one should be taken then?
Afterwards, the other method can be deleted.
|
|
||
| sort.Sort(labelSorter(labels)) | ||
| sort.Sort(labelSorter(milestone.Labels)) | ||
|
|
||
| var toAdd, toRemove []*Label | ||
|
|
||
| addIndex, removeIndex := 0, 0 | ||
| for addIndex < len(labels) && removeIndex < len(milestone.Labels) { | ||
| addLabel := labels[addIndex] | ||
| removeLabel := milestone.Labels[removeIndex] | ||
| if addLabel.ID == removeLabel.ID { | ||
| // Silently drop invalid labels | ||
| if removeLabel.RepoID != milestone.RepoID && removeLabel.OrgID != milestone.Repo.OwnerID { | ||
| toRemove = append(toRemove, removeLabel) | ||
| } | ||
|
|
||
| addIndex++ | ||
| removeIndex++ | ||
| } else if addLabel.ID < removeLabel.ID { | ||
| // Only add if the label is valid | ||
| if addLabel.RepoID == milestone.RepoID || addLabel.OrgID == milestone.Repo.OwnerID { | ||
| toAdd = append(toAdd, addLabel) | ||
| } | ||
| addIndex++ | ||
| } else { | ||
| toRemove = append(toRemove, removeLabel) | ||
| removeIndex++ | ||
| } | ||
| } | ||
| toAdd = append(toAdd, labels[addIndex:]...) | ||
| toRemove = append(toRemove, milestone.Labels[removeIndex:]...) | ||
|
|
||
| if len(toAdd) > 0 { | ||
| if err = milestone.addLabels(db.GetEngine(ctx), toAdd, doer); err != nil { | ||
| return fmt.Errorf("addLabels: %v", err) | ||
| } | ||
| } | ||
|
|
||
| for _, l := range toRemove { | ||
| if err = milestone.removeLabel(db.GetEngine(ctx), doer, l); err != nil { | ||
| return fmt.Errorf("removeLabel: %v", err) | ||
| } | ||
| } | ||
|
|
||
| milestone.Labels = nil | ||
| if err = milestone.loadLabels(db.GetEngine(ctx)); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
That seems overly complicated for what you are trying to achieve.
I thought you wanted to clear all labels and set only the ones given as a parameter?
That can be achieved much more clearly: Drop all labels and then add all given labels to the milestone.
I refuse to review this method in the current implementation.
| // "$ref": "#/responses/empty" | ||
| // "403": | ||
| // "$ref": "#/responses/forbidden" | ||
| // "422": |
There was a problem hiding this comment.
Where does it return that?
The http.StatusUnprocessableEntity?
GiteaBot
added
the
lgtm/need 2
|
|
||
| for _, label := range labels { | ||
| // Silently drop invalid labels. | ||
| if label.RepoID != m.Repo.ID && label.OrgID != m.Repo.OwnerID { |
There was a problem hiding this comment.
As mentioned: Make that a method! I've seen it at list four times now.
| var labelIDs []int64 | ||
| var err error | ||
| selectLabels := ctx.FormString("labels") | ||
| if len(selectLabels) > 0 && selectLabels != "0" { |
There was a problem hiding this comment.
Why can't it be 0? Is such a label name prohibited?
| @@ -0,0 +1,9 @@ | |||
| <a | |||
There was a problem hiding this comment.
Is there a reason why you don't just use the template that is used for issue labels?
Because if there is not, then I'd recommend for maintainability purposes to use only the existing one.
| @@ -0,0 +1,11 @@ | |||
| <div class="ui labels list"> | |||
There was a problem hiding this comment.
Again, isn't there an issue equivalent you can use?
If yes, then please use that to improve future maintainability.
| @@ -393,8 +393,8 @@ function initSimpleMDEImagePaste(simplemde, dropzone, files) { | |||
| let autoSimpleMDE; | |||
|
|
|||
| function initCommentForm() { | |||
| if ($('.comment.form').length === 0) { | |||
| return; | |||
| if ($('.comment.form').length === 0 && $('.select-label').length < 1) { | |||
| @@ -39,6 +39,40 @@ | |||
| <label>{{.i18n.Tr "repo.milestones.desc"}}</label> | |||
| <textarea name="content">{{.content}}</textarea> | |||
| </div> | |||
| <div class="ui segment metas"> | |||
| <input id="label_ids" name="label_ids" type="hidden" value="{{.label_ids}}"> | |||
There was a problem hiding this comment.
Can't you reuse the template that is used for issues here as well? (If it isn't a template yet make it!)
Again, I don't see a reason why we should impact future maintainability if not necessary.
Co-authored-by: delvh <[email protected]>
|
@techknowlogick Are you still working on this? |
|
@MrHBS are you? |
Fix #17244
Marked as WIP as I haven't completed the UI yet, but the API and the structs/migrations are complete.
cc: @chriseaton