MM-17468: Accept a number of days instead of two date arguments in `/incident test bulk-data` #549

alankan886 · 2021-05-07T04:28:48Z

Summary

This PR is for simplifying two date arguments in /incident test bulk-data into one number argument. Instead of /incident test bulk-data 10 3 2020-01-31 2020-11-22 2, now it's /incident test bulk-data 10 3 342 2. And the number of days is expected to be larger than 0.

Ticket Link

Fixes mattermost/mattermost#17468

mattermod · 2021-05-07T04:28:52Z

Hello @alankan886,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project.

Please help complete the Mattermost contribution license agreement?
Once you have signed the CLA, please comment with /check-cla and confirm that the CLA check is green.

This is a standard procedure for many open source projects.

Please let us know if you have any questions.

We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team.

alankan886 · 2021-05-07T04:37:00Z

/check-cla

agarciamontoro · 2021-05-07T07:23:01Z

/update-branch

agarciamontoro · 2021-05-07T07:28:21Z

Thank you for your contribution, @alankan886 🎉 We will review it as soon as we can!

cpoile

Looks good to me from a code standpoint, thanks @alankan886 !

cpoile · 2021-05-07T12:28:04Z

server/command/command.go

 	if err != nil {
-		r.postCommandResponse(fmt.Sprintf("The provided value for the last possible date, '%s', is not a valid date. It needs to be in the format 2020-01-31.", params[3]))
+		r.warnUserAndLogErrorf("unable to parse in location on time.Now(): %v", err)


Suggested change

r.warnUserAndLogErrorf("unable to parse in location on time.Now(): %v", err)

r.warnUserAndLogErrorf("unable to parse in location on time.Now().AddDate: %v", err)

…gin-incident-collaboration

alankan886 · 2021-05-08T01:11:52Z

Thank you for the comment @cpoile! The suggested change is made and pushed.

agarciamontoro

Thank you, @alankan886, it looks great! 🎉

I left some suggestions below.

agarciamontoro · 2021-05-12T08:37:03Z

server/command/command.go

 		return
 	}

-	end, err := time.ParseInLocation("2006-01-02", params[3], time.Now().Location())
+	begin, err := time.ParseInLocation("2006-01-02", time.Now().AddDate(0, 0, -days).Format("2006-01-02"), time.Now().Location())


Can we simplify this?

Suggested change

begin, err := time.ParseInLocation("2006-01-02", time.Now().AddDate(0, 0, -days).Format("2006-01-02"), time.Now().Location())

begin := time.Now().AddDate(0, 0, -days)

Hello, not sure if this matters, but they are slightly different where time.ParseInLocation() would give 2021-05-12 00:00:00 (no specific time) and time.Now() would give 2021-05-12 19:53:00.596493 (specific time). If you don't mind the differences, I'm happy to change them. Let me know if you have any thoughts on this!

We do not need a high specificity here, as the dates will be picked randomly in the range of dates. So I'd vote for making the code more readable using just time.Now().

agarciamontoro · 2021-05-12T08:38:53Z

server/command/command.go

+		return
+	}
+
+	end, err := time.ParseInLocation("2006-01-02", time.Now().Format("2006-01-02"), time.Now().Location())


Same here

Suggested change

end, err := time.ParseInLocation("2006-01-02", time.Now().Format("2006-01-02"), time.Now().Location())

end := time.Now()

agarciamontoro · 2021-05-12T09:19:29Z

server/command/command.go

@@ -130,7 +130,7 @@ func getAutocompleteData(addTestCommands bool) *model.AutocompleteData {
 		testCreate.AddTextArgument("Name of the incident", "Incident name", "")
 		test.AddCommand(testCreate)

-		testData := model.NewAutocompleteData("bulk-data", "[ongoing] [ended] [begin] [end] [seed]", "Generate random test data in bulk")
+		testData := model.NewAutocompleteData("bulk-data", "[ongoing] [ended] [days] [seed]", "Generate random test data in bulk")
 		testData.AddTextArgument("An integer indicating how many ongoing incidents will be generated.", "Number of ongoing incidents", "")
 		testData.AddTextArgument("An integer indicating how many ended incidents will be generated.", "Number of ended incidents", "")
 		testData.AddTextArgument("Date in format 2020-01-31", "First possible creation date", "")


We also need to update these descriptions. We need to modify this one as shown here and remove the next line

Suggested change

testData.AddTextArgument("Date in format 2020-01-31", "First possible creation date", "")

testData.AddTextArgument("An integer n. The incidents generated will have a start date between n days ago and today.", "Range of days for the incident start date", "")

@justinegeffen may have a better wording for this.

agarciamontoro · 2021-05-12T09:24:14Z

@itao, I think it is ok getting rid of the old way of creating the test incidents and use instead just a number of days. What this means effectively is that it will be much easier to specify the number of days you want the incidents to go back (you just write that number), but also that it is now impossible to specify an end date, so the range will always be [n days ago, today], with the range always ending in the day the test command is executed.

Is that ok? Or do we want to maintain the old way of generating this data?

itao

This is awesome! It makes the command one-clickable in a playbook which is helpful for the customer demo playbook. Nice!

agarciamontoro · 2021-05-14T13:50:29Z

/update-branch

agarciamontoro

This is great, @alankan886, thank you very much! Skipping QA as it is an internal, testing command.

Change two dates args to a number of days

fde9ae0

Merge branch 'master' into master

d11d7e0

agarciamontoro requested review from cpoile and agarciamontoro and removed request for cpoile May 7, 2021 07:25

agarciamontoro added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 7, 2021

cpoile approved these changes May 7, 2021

View reviewed changes

alankan886 added 2 commits May 7, 2021 19:44

Improve error message for begin

2c7e4c7

Merge branch 'master' of https://github.com/alankan886/mattermost-plu…

ead68cc

…gin-incident-collaboration

agarciamontoro requested changes May 12, 2021

View reviewed changes

agarciamontoro requested a review from itao May 12, 2021 09:20

agarciamontoro added the 1: PM Review Requires review by a product manager label May 12, 2021

Update testData args and clean begin and end

befec73

itao approved these changes May 14, 2021

View reviewed changes

agarciamontoro self-requested a review May 14, 2021 13:22

agarciamontoro removed the 1: PM Review Requires review by a product manager label May 14, 2021

Merge branch 'master' into master

2647dd0

agarciamontoro approved these changes May 14, 2021

View reviewed changes

agarciamontoro added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 14, 2021

agarciamontoro merged commit 611fe34 into mattermost:master May 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-17468: Accept a number of days instead of two date arguments in `/incident test bulk-data` #549

MM-17468: Accept a number of days instead of two date arguments in `/incident test bulk-data` #549

alankan886 commented May 7, 2021

mattermod commented May 7, 2021

alankan886 commented May 7, 2021

agarciamontoro commented May 7, 2021

agarciamontoro commented May 7, 2021

cpoile left a comment

cpoile May 7, 2021

alankan886 commented May 8, 2021

agarciamontoro left a comment

agarciamontoro May 12, 2021

alankan886 May 13, 2021

agarciamontoro May 13, 2021

agarciamontoro May 12, 2021

agarciamontoro May 12, 2021

agarciamontoro commented May 12, 2021

itao left a comment

agarciamontoro commented May 14, 2021

agarciamontoro left a comment

	r.warnUserAndLogErrorf("unable to parse in location on time.Now(): %v", err)
	r.warnUserAndLogErrorf("unable to parse in location on time.Now().AddDate: %v", err)

	begin, err := time.ParseInLocation("2006-01-02", time.Now().AddDate(0, 0, -days).Format("2006-01-02"), time.Now().Location())
	begin := time.Now().AddDate(0, 0, -days)

	end, err := time.ParseInLocation("2006-01-02", time.Now().Format("2006-01-02"), time.Now().Location())
	end := time.Now()

	testData.AddTextArgument("Date in format 2020-01-31", "First possible creation date", "")
	testData.AddTextArgument("An integer n. The incidents generated will have a start date between n days ago and today.", "Range of days for the incident start date", "")

MM-17468: Accept a number of days instead of two date arguments in /incident test bulk-data #549

MM-17468: Accept a number of days instead of two date arguments in /incident test bulk-data #549

Conversation

alankan886 commented May 7, 2021

Summary

Ticket Link

mattermod commented May 7, 2021

alankan886 commented May 7, 2021

agarciamontoro commented May 7, 2021

agarciamontoro commented May 7, 2021

cpoile left a comment

Choose a reason for hiding this comment

cpoile May 7, 2021

Choose a reason for hiding this comment

alankan886 commented May 8, 2021

agarciamontoro left a comment

Choose a reason for hiding this comment

agarciamontoro May 12, 2021

Choose a reason for hiding this comment

alankan886 May 13, 2021

Choose a reason for hiding this comment

agarciamontoro May 13, 2021

Choose a reason for hiding this comment

agarciamontoro May 12, 2021

Choose a reason for hiding this comment

agarciamontoro May 12, 2021

Choose a reason for hiding this comment

agarciamontoro commented May 12, 2021

itao left a comment

Choose a reason for hiding this comment

agarciamontoro commented May 14, 2021

agarciamontoro left a comment

Choose a reason for hiding this comment

MM-17468: Accept a number of days instead of two date arguments in `/incident test bulk-data` #549

MM-17468: Accept a number of days instead of two date arguments in `/incident test bulk-data` #549