WIP: Add Labels to Milestones #17265

techknowlogick · 2021-10-08T05:36:24Z

techknowlogick commented

2021-10-08 05:36:24 +00:00

Marked as WIP as I haven't completed the UI yet, but the API and the structs/migrations are complete.

cc: @chriseaton

Fix #17244 Marked as WIP as I haven't completed the UI yet, but the API and the structs/migrations are complete. cc: @chriseaton

👍 1

delvh reviewed 2021-10-08 09:01:12 +00:00

models/migrations/v197.go

delvh commented

2021-10-08 07:52:55 +00:00

What do you mean by UNIQUE(s)?
Shouldn't it be UNIQUE(milestoneid_labelid) or something similar?

What do you mean by `UNIQUE(s)`? Shouldn't it be `UNIQUE(milestoneid_labelid)` or something similar?

models/milestone_label.go

delvh commented

2021-10-08 08:26:57 +00:00

Why are those lines necessary?
That goes far beyond deleting the relation and rather seems like a copy-paste error, doesn't it?

Why are those lines necessary? That goes far beyond deleting the relation and rather seems like a copy-paste error, doesn't it? ```suggestion ```

delvh commented

2021-10-08 08:32:52 +00:00

	if err = milestone.loadLabels(e); err != nil {

```suggestion if err = milestone.loadLabels(e); err != nil { ```

delvh commented

2021-10-08 08:36:14 +00:00

// ReplaceLabels removes all current labels and adds the given labels to the milestone.

```suggestion // ReplaceLabels removes all current labels and adds the given labels to the milestone. ```

delvh commented

2021-10-08 08:41:07 +00:00

```suggestion ```

models/milestone_label.go

						
				@ -0,0 +71,4 @@

					}

					// Do NOT add invalid labels

					if milestone.RepoID != label.RepoID && milestone.Repo.OwnerID != label.OrgID {

delvh commented

2021-10-08 08:13:21 +00:00

I can understand the first condition, but why the second condition?
And why is it combined with an &&?

I can understand the first condition, but why the second condition? And why is it combined with an `&&`?

models/milestone_label.go

						
				@ -0,0 +92,4 @@

					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) {

delvh commented

2021-10-08 08:15:35 +00:00

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.

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.

models/milestone_label.go

						
				@ -0,0 +110,4 @@

					if err != nil {

						return err

					}

					defer committer.Close()

delvh commented

2021-10-08 08:18:14 +00:00

Shouldn't the ctx be closed as well, or does the committer suffice?

Shouldn't the `ctx` be closed as well, or does the committer suffice?

models/milestone_label.go

						
				@ -0,0 +157,4 @@

				}

				func (milestone *Milestone) loadLabels(e db.Engine) (err error) {

					if milestone.Labels == nil {

delvh commented

2021-10-08 08:40:21 +00:00

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.

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.

models/milestone_label.go

						
				@ -0,0 +213,4 @@

					if err != nil {

						return err

					}

					defer committer.Close()

delvh commented

2021-10-08 08:35:14 +00:00

See above regarding ctx.

See above regarding `ctx`.

models/milestone_label.go

						
				@ -0,0 +293,4 @@

					milestone.Labels = nil

					if err = milestone.loadLabels(db.GetEngine(ctx)); err != nil {

						return err

					}

delvh commented

2021-10-08 08:45:51 +00:00

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.

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.

routers/api/v1/repo/milestone_label.go

delvh commented

2021-10-08 08:55:02 +00:00

	// summary: Drop all previous milestone labels and replace them with new labels

```suggestion // summary: Drop all previous milestone labels and replace them with new labels ```

routers/api/v1/repo/milestone_label.go

						
				@ -0,0 +143,4 @@

					//     "$ref": "#/responses/empty"

					//   "403":

					//     "$ref": "#/responses/forbidden"

					//   "422":

delvh commented

2021-10-08 08:58:46 +00:00

Where does it return that?
The http.StatusUnprocessableEntity?

Where does it return that? The `http.StatusUnprocessableEntity`?

delvh reviewed 2021-10-08 09:18:18 +00:00

models/issue_milestone.go

						
				@ -100,0 +116,4 @@

								return fmt.Errorf("addLabel [id: %d]: %v", label.ID, err)

							}

						}

					}

delvh commented

2021-10-08 09:04:18 +00:00

As mentioned: Make that a method! I've seen it at list four times now.

routers/web/repo/milestone.go

						
				@ -119,0 +128,4 @@

						}

					}

					labels, err := models.GetLabelsByRepoID(ctx.Repo.Repository.ID, "", db.ListOptions{})

delvh commented

2021-10-08 09:07:11 +00:00

Why can't it be 0? Is such a label name prohibited?

Why can't it be `0`? Is such a label name prohibited?

templates/repo/issue/milestone_labels/label.tmpl

						
				@ -0,0 +1,9 @@

				<a

delvh commented

2021-10-08 09:10:23 +00:00

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.

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.

templates/repo/issue/milestone_labels/labels_sidebar.tmpl

						
				@ -0,0 +1,11 @@

				<div class="ui labels list">

delvh commented

2021-10-08 09:13:57 +00:00

Again, isn't there an issue equivalent you can use?
If yes, then please use that to improve future maintainability.

Again, isn't there an `issue` equivalent you can use? If yes, then please use that to improve future maintainability.

templates/repo/issue/milestone_new.tmpl

						
				@ -40,2 +40,4 @@

									<textarea name="content">{{.content}}</textarea>

								</div>

								<div class="ui segment metas">

									<input id="label_ids" name="label_ids" type="hidden" value="{{.label_ids}}">

delvh commented

2021-10-08 09:17:38 +00:00

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.

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.

web_src/js/index.js

						
				@ -396,7 +396,7 @@ function initSimpleMDEImagePaste(simplemde, dropzone, files) {

				let autoSimpleMDE;

delvh commented

2021-10-08 09:16:04 +00:00

?

techknowlogick reviewed 2021-10-13 19:43:30 +00:00

models/milestone_label.go

						
				@ -0,0 +71,4 @@

					}

					// Do NOT add invalid labels

					if milestone.RepoID != label.RepoID && milestone.Repo.OwnerID != label.OrgID {