Initial implementation #6

levb · 2020-01-05T20:53:14Z

See https://github.com/mattermost/mattermost-plugin-solar-lottery/blob/dev/demo.md

To run the entire demo script on your local instance, type /lotto demo all, requires test-data.

/lotto info for current info.

Also refactored p.API dependencies into api.PluginAPI

- initial user.IsAvailable based on the calendar

- also `user [dis]qualify` now complete

Bumps [handlebars](https://github.com/wycats/handlebars.js) from 4.1.2 to 4.5.3. - [Release notes](https://github.com/wycats/handlebars.js/releases) - [Changelog](https://github.com/wycats/handlebars.js/blob/master/release-notes.md) - [Commits](handlebars-lang/handlebars.js@v4.1.2...v4.5.3) Signed-off-by: dependabot[bot] <[email protected]>

…bars-4.5.3 Bump handlebars from 4.1.2 to 4.5.3 in /webapp

levb · 2020-01-13T23:38:10Z

Thanks @crspeller for the review, it's a lot of lines, appreciate it!

Overall the CLI needs some more work to make it nice to use.

Noted, both structurally (command/flag set) and heuristically, like allowing just a space-separated rotation names and @usernames.

A glossary of terms (rotation, shift... etc) would be especially useful.

👍

Some work needs to be done around smaller rotations. Got some strange results (like the same user being scheduled repeatedly)

Yes, I saw the same thing just before the demo. Likely de-randomized something in the last refactoring of the scheduler; adding tests should help.

It would be nice if the "guess" was stable if no parameters changed.

👍 will add a --randomize or something for the rare other use case.

User vs admin commands and help would be nice.

Right. One obvious improvement begging to be made is the concept of rotation admin(s) but sysadmin vs user would be a great step forward, agreed.

Integration with the workflow plugin would be great.

Let's file additional enhancement issues for the necessary APIs?

levb · 2020-01-20T03:31:05Z

Filed #11 and #12 filed to address some of the concerns.

I could not reproduce the small rotation weirdness, will try again, will file as separate issues; also need more tests for fill now.

@crspeller @cpoile I'm submitting the feedback as a separate PR, since it's quite massive. It can be re-pointed at master if it makes it easier (lots of refactoring there)

* Refactored api file structure, api_ for wrappers * updated dependencies * PR Feedback: renamed event type other to personal * PR Feedback: ResolveRotationName to return ErrNotFound when no results * PR Feedback: nits and naming * PR Feedback: removed PostAction placeholder * Copyright (c) 2019-... * PR Feedback: comment in plugin/api.go * PR Feedback: nits * Refactored solar-lottery autofill, no tests yet * added weight tests * Added tests for solar-lottery needs, pickUser * test for fillOne, bunch of refactoring, fixes * More fillOne tests, fixes * Refactored MORE! * amend minor * amend minor * amend minor

cpoile

This is huge, I'm learning a lot.

These are notes I'm taking while working through the code.

It's interesting -- you're using fields on structs (like in api) to carry parameters into methods, kind of like a context in a request... Yah, that is what you're doing. You're treating slash commands like http requests. Hah, nice. Once I figured that out it became a lot clearer. Maybe we can rename it to make this a bit more clear on first sight? What you're doing feels like it's overloading the meaning of an api struct -- because in other places we've used just to hold api methods or helper methods. This is more like a context.

I'm using the demo code as examples -- if we're going to push to community we definitely need thorough explanations for the commands, and formatted examples. Like: Here are the steps to set up a team, set up an icebreaker event, set up a SET, give someone time off. The rotation commands are particularly difficult, I think.

Is there a way to specify a rotation that has randomness but makes sure that all eligible people are picked i times before someone is picked for their i+1th time? Because that's how I imagine the ice-breaker should work.

Guess will fail completely if it can't fill a rotation. Maybe it should try its best, because often we don't have a full icebreaker, or maybe SET is less one person.

Minor nits (todos):

the box profile pic is Jira's

server/plugin/plugin.go

server/command/command.go

server/api/level.go

server/command/user_unavailable.go

server/command/rotation_join.go

server/api/rotations.go

server/api/user.go

server/api/rotation.go

Co-Authored-By: Christopher Poile <[email protected]>

levb · 2020-01-22T20:06:07Z

This is huge, I'm learning a lot.

Thanks, I did myself in the process. Go is very different from C where it comes to packaging code.

It's interesting -- you're using fields on structs (like in api) to carry parameters into methods, kind of like a context in a request... Yah, that is what you're doing. You're treating slash commands like http requests. Hah, nice. Once I figured that out it became a lot clearer. Maybe we can rename it to make this a bit more clear on first sight? What you're doing feels like it's overloading the meaning of an api struct -- because in other places we've used just to hold api methods or helper methods. This is more like a context.

My thinking - I'd like us to converge at some point in the future where there is a structure like

api - core logic
remote/... - anything with upstream backends (including workflow client-plugin-side code?)
store - persistence (basic, NO logic, mostly for easier mocking)

HTTP and Command interfaces really ought to become one at some point. I'd love for a senior community member/candidate to write an AST-based generator for these, with logging and all. With some creativity.

I am also hesitant to do much there until the HTTP endpoints are actually written and used - we'll see the best packaging then? I don't like the use of gorilla/mux for instance; other things aside it necessitates playing games with Context. One way would be to just rely on Context for everything but my understanding was that it was discouraged when it was designed. Anyway, I feel like it's something to solve later.

I'm using the demo code as examples -- if we're going to push to community we definitely need thorough explanations for the commands, and formatted examples. Like: Here are the steps to set up a team, set up an icebreaker event, set up a SET, give someone time off. The rotation commands are particularly difficult, I think.

I filed #11 to create a README. I agree that the documentation and "template use cases" are needed.

As far as improving the overall usability of the CLI, I suggest we wait until we gather experience on community. Having said that, there is one obvious improvement I'd like to make, and that is to rely on parameters (not flags) for rotation, user, and shift references, via a normalized syntax. Anything that is a @.+ is a user, anything that is a \w#[0-9]+ is a shift reference, anything else is a regexp on the rotation name. I'll think about it some more and file a HW ticket.

FWIW, I have mixed feelings at best about pflag, but nothing specific yet.

Is there a way to specify a rotation that has randomness but makes sure that all eligible people are picked i times before someone is picked for their i+1th time? Because that's how I imagine the ice-breaker should work.

No, but there is now separation of concern that should allow for an easy development of a queue Autofiller, which can keep its own persistence, and have its own logic if it wants to be smarter than a simple queue.

Separately, the current userWeight function in solarloterry only takes the LastServed into account. Once user's history is saved (a large-size new ticket?) it should be fully taken into account for scoring. I am still researching and thinking about how to best implement that. It may be the single best remaining improvement to the algorithm itself; its efficiency matters less than fairness IMO.

the box profile pic is Jira's

#16

levb · 2020-01-22T20:15:34Z

@cpoile @crspeller I think I addressed both of yours feedback. I took the liberty of resolving the threads that I felt were done, to keep the PR manageable, feel free to reopen as appropriate.

I filed a bunch of follow up tickets, but the biggest obstacles to the community deployment in my mind are:

Write a README #11 - may start working with 1-2 perspective POC "rotation managers" on it. Maybe @lindy65 and maybe a rotation in our team (me). Please feel free to comment in the ticket and/or contribute, based on your first-hand experience.
Add test mode to rotation #15 - to be able to test live rotations without spamming people.
Cron: implement POC (HA-aware) #2 (maybe just dithered, not truly HA-proof). May be ok to run autopilot manually at first though.
Graphic designers that are fans of Philip K. Dick? Need a bot icon. #16, of course!

crspeller

Lots of little comments. The biggest thing is some architectural changes we chatted about. I tagged some of them with inline comments.

Lots of stuff to like in here! The server could use the expand semantics....

Will take another look after the big changes.

server/api/api.go

server/http/http.go

server/plugin/plugin.go

server/command/rotation_add.go

crspeller · 2020-01-24T01:55:30Z

server/store/rotation_store.go

+
+	// Mandatory attributes
+	Name   string
+	Period string


Why not use time.Duration for this?

This one is tricky. To start with, a "month" is not a duration. I am treating weeks like they are, but they really are not and all these should be treated similarly, by properly calculating start/end using the calendar math.

server/store/rotation_store.go

server/api/period.go

server/api/shift_api.go

levb · 2020-01-24T05:47:18Z

@crspeller, done.

The Autofiller interface has to be defined in the solarlottery package, or it will create a circular dependency - it uses sl entities as parameters, and is needed by sl itself.
I did create the tree though, moved Error there, much nicer. Also added a queue placeholder to show the intent. Also, since the top package name is always aliased to keep it short, I don't think duplicate names are an issue, thoughts?
Thanks for suggesting renaming the test files, I ended up restructuring the code instead to match the tests, looks better now.
Period/Duration is not a trivial matter, see my inline comment. Suggest leaving as is.

cpoile

From the review last week, I can't see anything I would really want changed. We'll have to let it bake/shake-out on community for a bit.

crspeller

We could nitpick on this forever. It's a lot of code, so let's get it merged and start iterating on it. Once we get some real feedback we can improve the design. I think the overall design is good and it's already staring to bleed into our other plugins :D

levb · 2020-02-06T14:20:18Z

I'm going to merge to master as is, and log a PM review ticket for the community deployment milestone, specifically after there's a README.

Lev Brouk and others added 30 commits December 7, 2019 13:49

Initial builds

8baba24

/slottery skills

6d18366

/slottery rotation

8de2b89

Added join/leave and user show commands

500b52f

/user skill command

375b3e7

Added rotation needs editing command switches

ee44715

Shift entity and the beginning of a scheduler

5d8f2a4

First test for Solar Lottery passes

2b996f7

Added a simple even distribution test

de3b2e8

http and command to the top; style

f17bf83

Added --user to join/leave

a860477

Also refactored p.API dependencies into api.PluginAPI

WIP /slottery rotation show --schedule - wip

f55d6c5

/slottery forecast - refactored

a37a0ba

/shift and /volunteer initial implementation

88bce4a

refactored commands, round 0

6a1300d

- initial user.IsAvailable based on the calendar

Added user unavailable command

1571c8d

- also `user [dis]qualify` now complete

wip forecast appears to work on simple

c104473

Refactored command, still need shift, user forecast

de2eff3

wip more refactoring

f918e6c

wip more shift command, fixed backfill

a257227

Added shift fill command, style

c2709bf

Added user forecast

f0bcfb7

Merge pull request #1 from levb/dependabot/npm_and_yarn/webapp/handle…

b7dd165

…bars-4.5.3 Bump handlebars from 4.1.2 to 4.5.3 in /webapp

github webapp vulnerabilities

3f230fe

Shift start works

9ba2ea3

wip autopilot API

bebffff

wip added rotation autopilot command

e67d61b

wip many shift changes, forecast works, autopilot untested

1101c1b

wip refactored user messages, minor fixes

b010bf5

levb requested review from aaronrothschild and crspeller and removed request for aaronrothschild January 20, 2020 03:40

cpoile reviewed Jan 21, 2020

View reviewed changes

levb and others added 6 commits January 22, 2020 10:20

Update server/command/user_unavailable.go

e0d5776

Co-Authored-By: Christopher Poile <[email protected]>

Update server/command/rotation_join.go

29f21f9

Co-Authored-By: Christopher Poile <[email protected]>

Update server/api/user.go

f7a9d1e

Co-Authored-By: Christopher Poile <[email protected]>

Update server/command/user_qualify.go

901c8f0

Co-Authored-By: Christopher Poile <[email protected]>

PR Feedback, style

0c19a1f

PR Feedback: nits

cd078df

levb requested a review from cpoile January 22, 2020 20:06

crspeller requested changes Jan 24, 2020

View reviewed changes

PR Feedback: api->solarlottery, http->api

b78bae5

levb requested a review from crspeller January 24, 2020 05:47

cpoile approved these changes Feb 4, 2020

View reviewed changes

crspeller approved these changes Feb 5, 2020

View reviewed changes

levb added 4: Reviews Complete All reviewers have approved the pull request and removed 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Feb 6, 2020

levb added this to the v0.1 - community milestone Feb 6, 2020

levb merged commit 58994fe into master Feb 6, 2020

levb deleted the dev branch February 6, 2020 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation #6

Initial implementation #6

levb commented Jan 5, 2020 •

edited

Loading

levb commented Jan 13, 2020

levb commented Jan 20, 2020

cpoile left a comment •

edited

Loading

levb commented Jan 22, 2020

levb commented Jan 22, 2020 •

edited

Loading

crspeller left a comment

crspeller Jan 24, 2020

levb Jan 24, 2020

levb commented Jan 24, 2020

cpoile left a comment

crspeller left a comment

levb commented Feb 6, 2020

Initial implementation #6

Initial implementation #6

Conversation

levb commented Jan 5, 2020 • edited Loading

levb commented Jan 13, 2020

levb commented Jan 20, 2020

cpoile left a comment • edited Loading

Choose a reason for hiding this comment

levb commented Jan 22, 2020

levb commented Jan 22, 2020 • edited Loading

crspeller left a comment

Choose a reason for hiding this comment

crspeller Jan 24, 2020

Choose a reason for hiding this comment

levb Jan 24, 2020

Choose a reason for hiding this comment

levb commented Jan 24, 2020

cpoile left a comment

Choose a reason for hiding this comment

crspeller left a comment

Choose a reason for hiding this comment

levb commented Feb 6, 2020

levb commented Jan 5, 2020 •

edited

Loading

cpoile left a comment •

edited

Loading

levb commented Jan 22, 2020 •

edited

Loading