google/peoplemath

Web application for team planning

peoplemath's Issues

Consider ways to indicate that an objective depends on other teams

Some objectives can be completed with little or no assistance from other teams, but others cannot.

It would be good to think about whether there are some simple ways to model and show the extent to which an objective depends on other teams allocating people over the same period. Possibly just a boolean for "high" versus "low" external dependencies, displayed as a slightly different background colour?

Concurrent modification check should take place inside a database transaction

The check of the LastUpdateUUID currently performed by the ensureNoConcurrentMod function in main.go should be performed in the same database transaction as the actual write of the Period. The transaction should be committed only if the UUID on the incoming object matches the one in the database.

Under the current implementation, the check is performed outside the database write transaction. That means there is still a race condition whereby two updates to the same Period take place concurrently, both having a correct LastUpdateUUID. If both updates pass the ensureNoConcurrentMod check before either is written to the database, both write transactions will succeed, and both update requests should receive a successful HTTP response code, when in fact one write has stomped over the other one.

The same is true of the checks in ensureTeamExistence and ensurePeriodExistence as well, though race conditions here are not as harmful, as these checks are repeated (in the current google_cds_store.go) inside the database transaction. The same thing could be done for the UUID check (i.e. verifying it both in main.go and a database transaction), though it would be better to simply perform all the checks once in the database layer where possible, and make this part of the semantics of the StorageService.

This would likely require modifying the StorageService interface to return custom error codes, so that the different types of error can be distinguished in main.go and suitable HTTP status codes returned to the client in the different cases.

Consider options for grouping and tagging objectives

Sometimes objectives belong together in the stack rank, e.g. when it makes no sense to complete one without the other during the period, but users prefer not to merge them into a single objective for clarity or brevity.

In such cases, the ability to group objectives together can be useful. E.g. if you want to move them in a block within the stack rank, it's inconvenient to do so by dragging them one at a time.

It's not immediately clear to me how this need could be addressed in the UI.

Show cumulative sum of estimates within bucket

It would be useful to see the cumulative sum of the effort estimates for each objective, in descending priority order within each bucket.

This would give a sense of which objectives it's likely to be possible to work on during the period, based on the resource limits for each bucket, without actually assigning the objectives.

The numbers could be shown on the right of each bucket, perhaps colour coded to show when the cumulative sum rises above the resource limit.

Renaming a group should only affect that group type

Currently, renaming group A to B in the breakdown for group type X will rename all groups named A to B. It should only rename groups of type X.

Add anchors and table of contents to period summary

Now the summary report has multiple panes, a table of contents and a set of internal anchor links would be very useful.

Prevent adding multiple users with the same ID

Currently, it is possible to add multiple users with the same ID. This should be prevented, as it causes unexpected consequences throughout the UI.

Performance analysis and optimisation

On the main period view, I see signs of unnecessary change detections making the app a little sluggish when there are lots of objectives.

For example, I see the "C" badges on committed objectives flashing when you press a key in the edit dialogs, and typing into text fields feels a bit sluggish.

There should be no need for change detection to run in these cases. We need to profile to check what's going on, then consider techniques for reducing the triggering of change detection and/or reducing DOM manipulation.

Make period edit toggle accessible regardless of scroll position

Currently, in the period view, it's necessary to scroll to the top to access the edit button and reorder toggle. Ideally these would be accessible from any scroll position.

Add the ability to delete a bucket

This is quite a destructive operation, so careful verification would be needed before actually doing it (especially if there are actual objectives in the bucket). But it's still something that needs to be done sometimes.

Improve display of assignments by person

Entries in the "assignments by person" section overlap each other once they get too long.

Period loadData function does not handle the no-auth use case

If authentication is turned off, the following code does not work:

peoplemath/src/app/period/period.component.ts

Line 283 in 5d7a3c7

team.teamPermissions.write.allow.forEach(permission => {

The allow array will be empty in this case, but that doesn't mean the user doesn't have permissions, because the whole auth feature is switched off.

Treating multiple adjacent objectives as a single block (e.g. for bulk reordering)

Sometimes objectives belong together in the stack rank, e.g. when it makes no sense to complete one without the other during the period, but users prefer not to merge them into a single objective for clarity or brevity.

In such cases, the ability to combine adjacent objectives into a single "block" would be useful. E.g. if you want to move them together within the stack rank, it's inconvenient to do so by dragging them one at a time.

It's not immediately clear to me how this need could be addressed in the UI.

How to host in my own server

Authentication and authorization support

Currently, PeopleMath has no support for authenticating users, or for restricting read or write access to any part of the application to certain groups of users.

I suspect many teams who might consider deploying this application outside Google would find this useful. Few would want the entire world to be able to read, let alone modify, all their data.

Many users would probably benefit from a more sophisticated access control scheme, where different teams have different groups of users who are able to read and write data associated with those teams.

Ideally the application should support a pluggable authentication scheme. The Google App Engine documentation suggests several options, but the implementation should not assume the application is running in Google App Engine; authentication should be abstracted such that you can drop in another mechanism if you want to without changing anything else.

Authorization should also be pluggable. The simplest implementation on App Engine would probably be to represent user groups as objects in Cloud Datastore, on a global and per-team basis (so a user would have access to a team if they are in either the relevant (read or write) global group, or the team-specific group). However, again, it should be possible to substitute another implementation for those not using App Engine. Some users, for example, may want to integrate with a role-based access control system inside their organisation.

Enable consistent renaming of groups and tags

Currently, if you want to rename a group or tag, the only way to do it is to open each objective with that group or tag and update the values one-by-one.

It would be good to have a way to consistently rename a group or tag across all objectives with a single action.

Make HTTP errors more noticeable

A couple of times I've accidentally dismissed the snackbar that tells you a concurrent edit has been made.

This error should be made more noticeable - perhaps a modal that can only be dismissed by clicking a certain button, and/or maybe pressing the escape key?

It should prompt the user to copy any unsaved work and then reload the page.

Printing only results in a single page

When you try to print either the period or period-summary view, you only get a single page, regardless of how much content there is.

The page you get aligns with the current browser view - in other words, if you're scrolled to the top of the page, you get what you'd normally expect to be the first page, but if you're scrolled further down, you'll get a single-page "window" on the middle of the document.

A little searching suggests this seems to be quite a common issue in Angular applications, and might be possible to fix with a few CSS changes. For example: angular/components#8378

Printing would be particularly valuable for the period summary view, as it would allow you to take a snapshot of that page, say in PDF format. However, hopefully fixing the underlying issue will make printing "just work" for all views.

Classify objectives as aspirational or committed

It should be possible to mark an objective as either "aspirational" or "committed", and have it labelled as such in the UI.

Migrate to App Engine Go 1.13 runtime

This runtime is available now:

https://cloud.google.com/appengine/docs/standard/go/runtime#go-1.13

Add Gorilla Web Toolkit

Docs https://www.gorillatoolkit.org/ .

This is flexible tool, this will allow us to create a separate route for each method (GET, POST) so we dont need to use if/else condition.

@amdw reply
Sounds good - the Gorilla libraries seem quite well-regarded, so if this simplifies the code then I'm all for it!

Make all component inputs immutable

It would make data flow within the application easier to reason about if all component inputs were immutable.

This would also allow change detection optimisations such as OnPush to be used (article).

The best way to enforce this would be to use actual immutable datatypes (such as the ones provided by Immutable) for the types of the component inputs.

Use smaller more consistent badge for committed objectives

The Angular Material purple "C" badge currently used to mark committed objectives is offset to the top left of each objective. This is how badges currently work in Angular Material.

Because of this, badges can overlap each other unless each objective is in a box with a lot of padding, such as a mat-list-item. This happens for example in the breakdowns by person, group and tag in the main period view, which is quite ugly. Because of this, a text "[C]" was used in the summary view.

What I'd prefer is to use a consistent marker for this throughout the app, something that is smaller and in line with the text. I think something like the badge in Bootstrap would work well:

https://getbootstrap.com/docs/4.4/components/badge/

I don't believe Angular Material supports anything like this right now, so would have to give some consideration to how best to implement it.

Add read and write access control lists for teams

Currently, period models can be edited by anyone able to make an HTTP request to the application backend. The application performs no authentication or authorization checks of its own, relying on a proxy server to apply such checks before requests reach it.

This makes it cumbersome to apply more sophisticated access controls, such as "group A may read and write periods for team X, while group B may only read them; group C may read and write periods for team Y".

The best solution I currently see would be to add fields like "readACL" and "readWriteACL" to the Team entity, indicating the lists of users (or groups) that can perform those actions to entities belonging to that team. Then, the application itself would need to perform authentication on all requests for those entities, checking that the requesting user is authorised before proceeding to serve the request.

To protect users from locking themselves out accidentally, an empty ACL (the default) should probably mean access is unrestricted. The UI should also probably warn the user if they are about to perform an action that would restrict their own access in some way.

The specific authentication mechanism used should be "pluggable", with the details hidden behind a Go interface, similar to the way the data storage mechanism is currently.

The ability to control access to the list of teams, and to the ability to add new teams, may also be useful.

Add location column to people table

For teams split across multiple locations or timezones, it may be helpful to have that information in the people table, so for example you can sort by it.

Enable secondary resource units

In some cases it may be useful to have additional resource units for a period, convertible to or from the primary unit using a fixed conversion factor.

For example, the main unit for a period might be person weeks, but it might also be interesting in places like the summary report to see figures in headcount, assuming one headcount is assumed to be a fixed number of person weeks.

Create new 'controllers' package

Create new package "controllers" and create 2 controllers files in it: team.go period.go. Inside controllers we can put handlers like Get() Post() Put() - i think it could be cool.
What do you think about it?

@amdw reply
Splitting the team-related and period-related controllers into separate files sounds reasonable to me if we can make it work. There may be some code shared between those controllers, but we can cross that bridge when we come to it.

Consider using an expandable tree to show objectives in period summary

Currently, there is a "show objectives" toggle in the group summary pane(s) in the period summary view, to allow the specific objectives to be displayed underneath each group.

It would be possible to use a tree for this instead, so that the user could expand individual groups to see the objectives. Angular Material has a tree component:

https://material.angular.io/components/tree/overview

The advantage of this would be that the user could easily open and close individual groups of interest. Currently, it's easy to lose your scroll position when you turn on the "show objectives" toggle, especially if there are a lot of groups and/or objectives.

The disadvantage of using the standard Angular Material tree component would be that the view would become much less information-dense: each tree node is much taller than an individual list item.

"Improve this" link target should be overridable

It should be possible to override the target of the "improve this" link via an environment property. This is so that teams can point it to internal resources with information about that specific instance.

Set up an automated continuous build

We would benefit from a continuous build for this project, which:

• Runs an Angular build (npx ng build --prod) to ensure there are no errors (and ideally warnings)
• Runs the Angular tests (npx ng test --browsers ChromeHeadless)
• Runs the Go backend tests (cd backend && go test ./...)

Ideally this would run automatically on every pull request.

Get rid of 'Server' struct

For now this entity contains store and defaultTimeout. defaultTimeout - I suggest to put it in "storage" package. StorageService can be placed globally as a variable in "storage" package. This improvement should give us "low-coupling", because storage and REST API actions are different abstractions in a programm.

Reply from @amdw
I'm open to suggestions on this, but I'm not generally a big fan of global variables. The fact that StorageService is behind an interface is already intended to provide loose coupling between the HTTP server and the storage implementation, and the storeTimeout (which determines how long the server is prepared to wait for the store to return a value) seems more a a concern of the HTTP server than the storage service. Having said that, I'm happy to look at what you have in mind.

Incorporate notes and action items

Some users would like to be able to associate notes and action items with their period models. Sometimes this information may be more confidential than the model itself, such as notes on team members' skills and abilities which led to the choice of assignments.

The simplest way to implement this would be to allow a link to be specified, that would create a "notes" or "action items" button in the period view that would open that URL in a new window. That link could be for example a Google Doc, which would provide access to all the features of G Suite, such as access controls and comments.

An alternative would be to add a "notes" field to each objective, and allow notes to be switched on and off in the view. The problem with this is that it would be another view "mode" that needs testing, and could lead to a more cluttered UI. The need for additional access controls would also make things more complex, both in the UI and the backend.

It is also not necessarily the case that all notes map directly to objectives: they may apply to groups of objectives or to the model as a whole. At the moment I favour adding a simple "notes" link to the period model as an easy-to-implement compromise.

Reading existing team without permissions results in nulls

Currently, when you run the app with existing teams in storage that don't have any permissions, you get an exception in the frontend here, because team.teamPermissions.write.allow is null:

peoplemath/src/app/period/period.component.ts

Line 283 in 5d7a3c7

team.teamPermissions.write.allow.forEach(permission => {

We need to scrub such periods here, to ensure the slice of matchers is never nil:

peoplemath/backend/google_cds_store/google_cds_store.go

Line 201 in 5d7a3c7

func scrubLoadedPeriod(period *models.Period) {

More user-friendly way of pushing a non-prod Angular build to App Engine

Right now build_appengine.sh passes --prod to ng build, and to change that you have to edit the script. It would be good to have a simpler way of doing this.

Support rich formatting (e.g. markdown) in objectives and their notes

There are occasions when it would be useful to have certain types of rich formatting, such as links or bold/italic text, in objectives and/or their notes.

This could be accomplished by supporting a markdown subset or some similar syntax. There is almost certainly already a suitable Javascript library for this.

Make it possible to delete a person

This will require a warning if the person has any assignments.

Move scrubbing functions into HTTP controllers and add tests

These functions should probably live in HTTP controllers, not in the Cloud Datastore code.

peoplemath/backend/google_cds_store/google_cds_store.go

Line 203 in c82a18b

// TODO Consider moving this function into the controller so it applies to all storage implementations.

This would mean the scrubbing would automatically happen regardless of the underlying storage implementation, and they would be easier to test as well.

Move permission-checking code into a separate type

This code is likely to be common to any Auth implementation which wants to perform actual permission checks. It therefore doesn't really belong in Firebase-specific code:

peoplemath/backend/auth/firebase.go

Line 68 in 2fcd365

// TODO The below code is likely to be the same for any Auth implementation which

Improved means of reordering objectives

Currently, reordering objectives is done using up and down buttons. This makes it hard to move an objective a long way (e.g. from the top to the bottom). Also, with a long objective name, the buttons wrap and overlap in an ugly way.

There should be a better way of doing this, e.g. drag-and-drop.

Redesign assignment dialog

The design of the dialog to assign people to objectives needs a bit of a rethink.

A few ideas:

• The "all" and "remaining" buttons are a bit confusing, and should probably be merged into a single button, which reassigns all the unassigned work to a given person (up to the maximum they have available).
• Buttons should be consistently greyed out when pressing them makes no sense. For example, it shouldn't be possible to press "+1" when an objective is fully assigned.
• We should consider not showing people who don't have any resources available, as it's not possible to usefully interact with these, and they just clutter the view.
• We should consider a way to get to the assignment dialog from the edit-objective dialog. For example currently, when an objective has an estimate of zero and you want to change that and then assign it, you have to click to edit the objective, change the estimate, then close the dialog (and save) and then press assign.

Grouping and tagging system

It would be useful to be able to group and tag objectives, in order to provide views that aggregate objectives together.

For example, objectives could be grouped into projects or products, so it's possible to see how much of a team's resources are being spent on each, and get a sense of the stack ranking of these aggregates.

Also, tags would be useful to provide highlighting of objectives with certain themes.

Grouping would be a mutually-exclusive concept: each objective could be a member of at most one group of each "group type" (e.g. "project").

Tagging, by contrast, would be a many-to-many concept: each objective could have multiple tags, and each tag could be applied to multiple objectives.

Consistently use server-side logic to determine permissions in the front-end

There are some places in the UI where we determine whether the current user has edit permissions, based on the contents of the teamPermissions. Examples:

peoplemath/src/app/period/period.component.ts

Line 284 in f56a7b6

// TODO Replace this logic with something determined by the server, like CanAddTeam

peoplemath/src/app/teamperiods/teamperiods.component.ts

Line 83 in f56a7b6

// TODO Replace this logic with something determined by the server, like CanAddTeam

This duplicates logic on the server side. For example, we need to look at environment.requireAuth to decide whether the server will even try to do any authentication at all.

There are places where we do make this kind of determination based on server-side logic, such as canAddTeam on the team list:

peoplemath/backend/controllers/team.go

Line 70 in 5d7a3c7

userCanAddTeam := s.auth.CanActOnTeamList(user, permissions, auth.ActionWrite)

We should use this technique consistently.

This may require backwards-incompatible changes to the JSON API, namely adding a wrapper object that contains the permissions and the team itself. But this is fine, as the backend is tightly coupled to the frontend at the moment (there is no mobile app whose versions can skew). We already made such a change to the team list.

Distinguish unassigned objectives from zero-effort ones

Currently, objectives show in grey if they are unassigned. This is intended to emphasize that these objectives will not be worked on during the period.

An objective with zero as its resource estimate is one which does not require any effort in the period; such an objective can never have any assignments and will always show in grey.

It would be good to distinguish these two types of objectives. Perhaps an even lighter shade of grey for zero-estimate objectives?

Fix flaky Angular tests

After setting up the continuous build, I noticed a few unrepeatable failures in the Angular tests. In a typical failed build, a single test would fail, with a stack trace like this:

Chrome Headless 85.0.4183.102 (Linux x86_64) ERROR
  An error was thrown in afterAll
  TypeError: this.storage.updatePeriod is not a function
      at <Jasmine>
      at PeriodComponent.performSave (http://localhost:9876/_karma_webpack_/src/app/period/period.component.ts:333:18)
      at SafeSubscriber._next (http://localhost:9876/_karma_webpack_/src/app/period/period.component.ts:67:75)
      at SafeSubscriber.__tryOrUnsub (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/Subscriber.js:183:1)
      at SafeSubscriber.next (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/Subscriber.js:122:1)
      at Subscriber._next (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/Subscriber.js:72:1)
      at Subscriber.next (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/Subscriber.js:49:1)
      at DebounceTimeSubscriber.debouncedNext (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/operators/debounceTime.js:40:1)
      at AsyncAction.dispatchNext (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/operators/debounceTime.js:53:1)
      at AsyncAction._execute (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/scheduler/AsyncAction.js:51:1)
      at AsyncAction.execute (http://localhost:9876/_karma_webpack_/node_modules/rxjs/_esm2015/internal/scheduler/AsyncAction.js:39:1)

I'm not sure exactly what's going on here, but this feels like it might be a race condition of some kind.

Add new storage type models logic to work with SQL server

You have relations in your models. Why did you choose NoSQL? Dont you want to create SQL store type just to try? I can do it also. I have created today docker env #42 so I can add PostgreSQL container with adminer.

@amdw reply
Regarding the choice of data store: a NoSQL database seemed like a good fit because the Period data structure is quite deeply nested (not having to do joins potentially brings some performance gains), and we have quite simple querying needs. The lack of schema makes it very easy to add new fields - you just modify the Go structs and everything just works.

The nice thing about Cloud Datastore specifically is that there is quite a generous free tier, meaning that you can run small instances on App Engine without paying anything, which is not the case even for the smallest SQL database (as Cloud SQL instances require a dedicated VM).

That said, I can well imagine that some users would prefer to use a SQL data store, and I'm quite happy to accept a new version of the StorageService that uses a SQL database instead, perhaps using Gorm or similar. The only thing is that I'm not likely to use it myself, so I can't promise to invest time in maintaining it.

Replace "see server log" in error messages

This assumes users will have access to the server logs, which won't be true in all cases.

We should consider other ideas, e.g. putting a UUID in the error message which the user can supply to the administrator of their instance, making it easy for them to find the right log entry.

Allow objectives to be moved from one bucket to another

Sometimes you want to move an objective from one bucket to another. It would be good to have a better way than deleting it from one bucket and then adding it back to another.

Moving an objective to another bucket is broken

It appears to delete the objective from the old bucket without adding it to the new bucket. In other words, it is currently functionally equivalent to delete. :(

Migrate to Angular 10

Angular 10 is now out. We should upgrade.

https://update.angular.io/#9.0:10.0l3

With this version, strict mode is now the default for new apps. We should try to opt into as many of these strict settings as possible.

Add keyboard shortcuts

Keyboard shortcuts could make several user journeys quite a bit easier, especially in the Period view when editing. A particular example would be adding a new objective, but editing, deleting and reordering objectives would also be worth considering.