Added Tags for Organizational Management #369

fenriskiba · 2020-06-02T12:49:18Z

Added Tags for Organizational Management

Developed in collaboration with @bradphilips and @nambup.

Description

Adding Tags in a many-to-many relationship with flag
Added tags query parameter to findFlags
Added findTags, createTag, deleteTag, findAllTags API calls to manage Tags
Added option to postEvaluationBatch to evaluate for all Flags with provided Tags
Added unit and integration tests for Tags
Added elements in the UI to search, edit, and view Tags on a Flag

Motivation and Context

Please see Issue #362

How Has This Been Tested?

Unit Tests
Integration Tests
Tested in Local
Tested in internal sandbox environment

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

codecov-commenter · 2020-06-02T14:24:51Z

Codecov Report

Merging #369 into master will increase coverage by 1.88%.
The diff coverage is 93.42%.

@@            Coverage Diff             @@
##           master     #369      +/-   ##
==========================================
+ Coverage   81.38%   83.26%   +1.88%     
==========================================
  Files          27       27              
  Lines        1606     1733     +127     
==========================================
+ Hits         1307     1443     +136     
+ Misses        222      210      -12     
- Partials       77       80       +3

Impacted Files	Coverage Δ
pkg/entity/db.go	`76.47% <ø> (ø)`
pkg/handler/eval_cache_fetcher.go	`83.78% <81.81%> (+1.43%)`	⬆️
pkg/handler/crud.go	`89.03% <91.66%> (+0.65%)`	⬆️
pkg/handler/eval.go	`86.80% <91.89%> (+9.34%)`	⬆️
pkg/entity/fixture.go	`100.00% <100.00%> (ø)`
pkg/entity/flag.go	`95.45% <100.00%> (+1.16%)`	⬆️
pkg/handler/eval_cache.go	`82.14% <100.00%> (+7.14%)`	⬆️
pkg/handler/handler.go	`83.33% <100.00%> (+1.33%)`	⬆️
pkg/util/util.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bd88aa...b8dc36f. Read the comment docs.

zhouzhuojie · 2020-06-04T17:03:25Z

browser/flagr-ui/src/components/Flags.vue

        );
      }
      return this.flags;
    }
  },
  methods: {
+    tagFormatter(row, col, val) {
+      return val.map(tag => tag.value).join(', ');


maybe we can use el-tag for showing tags

Yeah we'll work on getting this working 👍

zhouzhuojie · 2020-06-04T17:10:14Z

browser/flagr-ui/src/components/Flag.vue

+                        v-model="newTag.value"
+                        ref="saveTagInput"
+                        size="mini"
+                        :trigger-on-focus="false"


I think if we set trigger-on-focus to true, it helps to standardize tags creation and reduce typos

@zhouzhuojie this was set to false because focus was triggering a save operation which we didn't want, requiring the saveTagInput method have to handle or ignore values that are empty but would still cancel your operation when tabbed away. This conflicts with entry process we're using with the save on enter and cancel.

zhouzhuojie · 2020-06-04T17:12:15Z

browser/flagr-ui/src/components/Flag.vue

+    },
+    loadAllTags() {
+      Axios.get(
+        `${API_URL}/tags`


I noticed that even when we delete the tag from across flags, it still shows up in the autocomplete.

This is actually intended for the Flags delete, because it simply removes the reference to the Tag from the Flag and not the actual Tag itself because it could be referenced by another Flag. Let us know if there's another way you'd like to handle this.

@zhouzhuojie, I was talking to @bradphilips and a couple of our other Flagr users, and we wondered if this would be a preferred way to handle this anyway. It would keep us from creating and deleting the same tag over and over if it goes in and out of use.

Brad also mentioned an option to search for any flags with the removed tag and fully delete the tag if there aren't any. I can see an argument for doing that to prevent unused tags from sticking around.

Personally, I like leaving them the way it's currently set up, but I don't think we'd argue if you want to fully delete the tags.

thanks, lgtm

zhouzhuojie · 2020-06-04T17:35:59Z

pkg/handler/eval_cache.go

+
+	results := []*entity.Flag{}
+	for _, t := range tags {
+		s := util.SafeString(t)


t is already a string type, so no need to cast to string here

Perfect, we'll remove this 👍

zhouzhuojie · 2020-06-04T17:44:22Z

pkg/handler/eval_cache.go

+		}
+	}
+
+	return FlattenFlags(results)


I can see that you are doing dedup here. Since flag id order doesn't matter, how about we define

tagCache: map[string]map[uint]*entity.Flag // i.e. tag => map[flagID]*entity.Flag

And when loop through the tags, we just need to get a union of the maps, and then return the values of that union map as a slice

zhouzhuojie · 2020-06-04T17:50:12Z

pkg/handler/eval.go

@@ -39,7 +39,8 @@ func (e *eval) PostEvaluation(params evaluation.PostEvaluationParams) middleware
 			ErrorMessage("empty body"))
 	}

-	evalResult := EvalFlag(*evalContext)
+	flag := LookupFlag(*evalContext)
+	evalResult := EvalFlagWithContext(flag, *evalContext)


since the function EvalFlag is still there and flag variable is not used below the line, I think you can keep evalResult := EvalFlag(*evalContext)

zhouzhuojie · 2020-06-04T17:57:14Z

pkg/entity/tag.go

+type Tag struct {
+	gorm.Model
+
+	Value string  `sql:"type:varchar(64);unique_index:idx_tag_value"`


Thinking about the validation of Value field, I think it's probably safer to start with stricter regex pattern, as the tag may grow with inconsistent naming pattern. e.g. eng_team_abc vs Eng Team EFG. A good starting point is util.IsSafeKey.

In the future, we can decouple the validation of tags with loose restrictions if needed.

@zhouzhuojie Makes perfect sense, could util.IsSafeKey be re-used here or should we develop an alternate method with our own requirements?

Added validation for this in the last commit. I copied and modified the regex from the IsSafeKey slightly to allow for spaces. Let me know if this is to your liking. Thanks!

…gic and removed SafeString

bradphilips · 2020-06-05T19:53:45Z

@zhouzhuojie Made some changes for items 1,4,5 and 6. Let us know what you'd like to do on the others. Thanks!

zhouzhuojie

Left a few comments, and awesome work! Thanks for the contribution. I will take a look again soon.

zhouzhuojie · 2020-06-11T16:44:11Z

browser/flagr-ui/src/components/Flag.vue

+    },
+    loadAllTags() {
+      Axios.get(
+        `${API_URL}/tags`


thanks, lgtm

zhouzhuojie · 2020-06-11T17:23:33Z

docs/api_docs/bundle.yaml

+      operationId: findAllTags
+      parameters:
+        - in: query
+          name: limit


nit, we may also want to add offset to enable pagination

zhouzhuojie · 2020-06-11T17:30:31Z

pkg/handler/crud.go

+
+	if err := getDB().Find(s).Association("Tags").Delete(t).Error; err != nil {
+		return tag.NewDeleteTagDefault(500).WithPayload(ErrorMessage("%s", err))
+	}


You may also need entity.SaveFlagSnapshot() to capture tag deletion in flag history.

zhouzhuojie · 2020-06-11T17:32:03Z

pkg/handler/crud_test.go

+		},
+	})
+
+	// step 0. it should return 0 tags before creaetion


can you also add some tests for limit and tags like parameter?

zhouzhuojie · 2020-06-11T17:39:52Z

pkg/handler/eval_cache.go

+	return values(results)
+}
+
+func values(m map[uint]*entity.Flag) []*entity.Flag {


I think the function names values and merge are too generic inside the handler package, and these two functions are simple enough and I think it's better to just inline into the GetByTags function.

… values and merge methods.

bradphilips · 2020-06-11T20:59:02Z

Left a few comments, and awesome work! Thanks for the contribution. I will take a look again soon.

Awesome, thank you @zhouzhuojie -- Fixed all the additional comments as well. Let us know if there's anything additional.

bradphilips · 2020-07-08T17:01:19Z

@zhouzhuojie thanks so much for the merge. Do you have an idea of when this will get into a release? We're eager to deploy this in our infrastructure! Thanks!

bradphilips · 2020-07-08T17:23:41Z

@zhouzhuojie thanks so much for the merge. Do you have an idea of when this will get into a release? We're eager to deploy this in our infrastructure! Thanks!

I see that it was just done. Thank you! :)

fenriskiba added 2 commits June 1, 2020 15:45

Added Tags for Organizational Management

cc0f346

Fix integration issue with mysql

e7937ce

zhouzhuojie reviewed Jun 4, 2020

View reviewed changes

Changes from review. Fixed tag cache, el-tags on Flags.vue, lookup lo…

31f20d0

…gic and removed SafeString

Added validation for Tag Value

3d94f5f

zhouzhuojie reviewed Jun 11, 2020

View reviewed changes

Added offsest, SaveSnapshot, added tests for FindAllTags, and removed…

b8dc36f

… values and merge methods.

zhouzhuojie approved these changes Jun 13, 2020

View reviewed changes

zhouzhuojie merged commit 41d2757 into openflagr:master Jun 13, 2020

fenriskiba mentioned this pull request Sep 8, 2020

"fatal error" when calling Batch Eval with Tags at Volume #398

Closed

zhouzhuojie mentioned this pull request Oct 6, 2020

[feat] Support evaluation for all enabled flags in 1 call #406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Tags for Organizational Management #369

Added Tags for Organizational Management #369

fenriskiba commented Jun 2, 2020

codecov-commenter commented Jun 2, 2020 •

edited

Loading

zhouzhuojie Jun 4, 2020

bradphilips Jun 4, 2020

zhouzhuojie Jun 4, 2020

bradphilips Jun 4, 2020 •

edited

Loading

zhouzhuojie Jun 4, 2020

bradphilips Jun 4, 2020

fenriskiba Jun 8, 2020

zhouzhuojie Jun 11, 2020

zhouzhuojie Jun 4, 2020

bradphilips Jun 4, 2020

zhouzhuojie Jun 4, 2020

zhouzhuojie Jun 4, 2020

zhouzhuojie Jun 4, 2020

bradphilips Jun 4, 2020

bradphilips Jun 8, 2020 •

edited

Loading

bradphilips commented Jun 5, 2020

zhouzhuojie left a comment

zhouzhuojie Jun 11, 2020

zhouzhuojie Jun 11, 2020

zhouzhuojie Jun 11, 2020

zhouzhuojie Jun 11, 2020

zhouzhuojie Jun 11, 2020

bradphilips commented Jun 11, 2020

bradphilips commented Jul 8, 2020

bradphilips commented Jul 8, 2020

Added Tags for Organizational Management #369

Added Tags for Organizational Management #369

Conversation

fenriskiba commented Jun 2, 2020

Added Tags for Organizational Management

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

codecov-commenter commented Jun 2, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradphilips Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradphilips Jun 8, 2020 • edited Loading

Choose a reason for hiding this comment

bradphilips commented Jun 5, 2020

zhouzhuojie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradphilips commented Jun 11, 2020

bradphilips commented Jul 8, 2020

bradphilips commented Jul 8, 2020

codecov-commenter commented Jun 2, 2020 •

edited

Loading

bradphilips Jun 4, 2020 •

edited

Loading

bradphilips Jun 8, 2020 •

edited

Loading