Workitem crud

First version: allows to read a single work item and a single work item type.

This change is

Nice. Missing some tests tho :)

Yes, i don't know how to write them yet
Am 23.06.2016 6:13 nachm. schrieb "Aslak Knutsen"<EMAIL_ADDRESS>
:

Nice. Missing some tests tho :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/almighty/almighty-core/pull/4#issuecomment-228101552,
or mute the thread
https://github.com/notifications/unsubscribe/AMjc-gYzSjwiUUjlFRSOyQrAeGHVv6Orks5qOrCUgaJpZM4I8524
.

Look at https://github.com/almighty/almighty-core/blob/master/version_test.go
Goa generates up Test helpers that can be found in app/test/ for all API entry points and return codes etc.

Since the status isn't updated, I try to trigger jenkins manually through these two words: test this

Review status: 0 of 41 files reviewed at latest revision, 3 unresolved discussions.

Makefile, line 7 [r7] (raw file):

endif

export DBHOST

Why does the DBHOST has to be exported? Is export even the correct way to do it on Windows? I thought something like set or env are the tools to use on Windows.

Comments from Reviewable

Review status: 0 of 41 files reviewed at latest revision, 3 unresolved discussions.

workitem.go, line 39 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> DB operations should be extracted out into some 'service' interface.

But you don't mean like a wrapper to SQL, do you? Just some service that knows PostgreSQL, right?

Comments from Reviewable

Reviewed 7 of 24 files at r1, 10 of 17 files at r3, 1 of 2 files at r4, 16 of 19 files at r5, 2 of 5 files at r6, 5 of 5 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

workitem.go, line 39 [r7] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> But you don't mean like a wrapper to SQL, do you? Just some service that knows PostgreSQL, right?

I mean a 'Planner API' e.g.
// Ish..
type Planner struct {
	db *gorm.DB
	Items Item
}

type Item struct {
	db *gorm.DB
}

func (i *Item) GetItem(id string) (*WorkItem, error) {
	db. .... do stuff
}

// Usage

func (c *WorkitemController) Show(ctx *app.ShowWorkitemContext) error {
	c.Planner.Items.GetItem(ctx.ID)
}

Comments from Reviewable

workitem.go, line 39 [r7] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> That probably makes sense. DAO-Style?

Repository

Comments from Reviewable

build/Makefile.lnx, line 6 [r7] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> Part of trying the makefile local to the project/workspace

Ok, this can be removed when go-vendor branch goes upstream then.

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

models/workitem.go, line 10 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> What is the intention with Name of a WorkItem? Are you thinking UserStory, Epic etc? If so, I was expecting that to be the 'DisplayName' or similar of the WorkItemType?

I agree

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Makefile, line 7 [r7] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> Why does the `DBHOST` has to be exported? Is `export` even the correct way to do it on Windows? I thought something like `set` or `env` are the tools to use on Windows.

export in makefiles makes the variable available in tools started from the makefile.

models/workitem.go, line 10 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> What is the intention with Name of a WorkItem? Are you thinking UserStory, Epic etc? If so, I was expecting that to be the 'DisplayName' or similar of the WorkItemType?

No, something that identifies the item to the user "Debugger crashes when setting breakpoint"

Comments from Reviewable

Review status: all files reviewed at latest revision, 3 unresolved discussions.

workitem.go, line 39 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> Repository

What's the difference, in your view?

Comments from Reviewable

workitem.go, line 39 [r7] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> What's the difference, in your view?

From an impl perspective not much. But DAO was originally intended to abstract away different Databases. With the rise of 'EntityManager' etc, the EntityManager became the DAO. The Repository abstracts away storage backend.  But yeah, details. Any Typed interface that abstracts away how to fetch the data is the point.

Comments from Reviewable

models/workitem.go, line 10 [r7] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> No, something that identifies the item to the user "Debugger crashes when setting breakpoint"

You mean 'Title/Subject'? Why isn't that part of the Fields definition of a WorkItem?

Comments from Reviewable

design/resources.go, line 31 [r7] (raw file):
Previously, bartoszmajsak (Bartosz Majsak) wrote…
> I believe this should return `CREATED` (201) and ideally `href` to this resource as part of the reponse.

201 is the normal response code used with a Location header pointing to the created resource. See the Goa overview "Responses" for how you can create a ResponseTemplate that covers this situation: http://goa.design/design/overview.html

Comments from Reviewable

Review status: 37 of 42 files reviewed at latest revision, 4 unresolved discussions.

models/workitem.go, line 10 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> You mean 'Title/Subject'? Why isn't that part of the Fields definition of a WorkItem?

Outside of fields are things that the work item service needs to function. Not quite sure yet where to draw the line.

Comments from Reviewable

models/workitem.go, line 10 [r7] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> Outside of fields are things that the work item service needs to function. Not quite sure yet where to draw the line.

Besides how you've built it currently; Why does the WorkItem Service require a Title?

Comments from Reviewable

Reviewed 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

design/resources.go, line 31 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> @tsmaeder Could you create an issue to track this?

https://github.com/almighty/almighty-core/issues/29

Comments from Reviewable

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

models/workitem.go, line 10 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> Besides how you've built it currently; Why does the WorkItem Service require a Title?

My thinking was that it would be usefule to have something that identifies a work item at a user level and that this be a "system" field. But perhaps I've listened to max too much ;-)

Comments from Reviewable

models/workitem.go, line 10 [r7] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> My thinking was that it would be usefule to have something that identifies a work item at a user level and that this be a "system" field. But perhaps I've listened to max too much ;-)

hehe, probably. :) Keep in mind that a WorkItem is more than Planner related. e.g. Not sure a Build has a Name? A Commit? A PullRequest Response? A Build Request? etc..

Comments from Reviewable

Reviewed 5 of 5 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

Reviewed 7 of 24 files at r1, 10 of 17 files at r3, 1 of 2 files at r4, 16 of 19 files at r5, 3 of 5 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

Review status: 7 of 27 files reviewed at latest revision, 17 unresolved discussions.

workitem.go, line 14 [r13] (raw file):

)

// WorkitemController implements the workitem resource.

Side note: If you're not happy with how Go forces you to comment on structs, be aware that it allows an article like A or The in front of the  struct's name in the comment. Often this makes much nicer sentences :)

workitem.go, line 29 [r13] (raw file):

}

// Show runs the show action.

Hm, can you elaborate on this please?

workitem.go, line 34 [r13] (raw file):

	idVal, err := strconv.Atoi(ctx.ID)
	if err != nil {
		return ctx.BadRequest(goa.ErrBadRequest("could not parse"))

Maybe could not parse ID of workitem is a better error?!

workitem.go, line 43 [r13] (raw file):
This is just a comment on code style.

wiType := loadTypeFromDB(res.Type)
result, err := convertFromModel(*wiType, res)

I think I read somewhere that it is good Go style to omit the first line and instead write
result, err := convertFromModel(*loadTypeFromDB(res.Type), res)

instead.

workitem.go, line 60 [r13] (raw file):

	for name, field := range wiType.Fields {
		var err error
		result.Fields[name], err = field.ConvertFromModel(name, workItem.Fields[name])

You can still write := here and omit the var err error. := also works if only part of the left hand side is undefined.

workitem.go, line 81 [r13] (raw file):

		fieldValue := ctx.Payload.Fields[fieldName]
		var err error
		wi.Fields[fieldName], err = fieldDef.ConvertToModel(fieldName, fieldValue)

You can still write := here and omit the var err error. := also works if only part of the left hand side is undefined.

workitem.go, line 86 [r13] (raw file):

		}
	}
	tx := c.db.Begin()

I'm thinking for concurrency reasons we should maybe do it this way:
var err error
tx := c.db.Begin()
defer func() {
	if err != nil {
		tx.Rollback()
	}
}()

This way we just return in case of errors below and don't care about manual rollbacks. At least in my opinion this is safer and more elegant. What do you think? The only issue I see is that you may want to roll back without having err set to non nil. On example is this line in Update method:
if res.Version != ctx.Payload.Version {`

You rollback in this if with err being nil.
But still I like the concept of having a safety net with defer.
Here's another idea where I substitute the err with a boolean that is always true unless we actually commit the tx:
rollback := true
tx := c.db.Begin()
defer func() {
	if rollback == true {
		tx.Rollback()
	}
}()

//...

rollback = false
tx.Commit()

Now, that looks much more promising because the assumption is defensive and assumes an error rather than success.
The same applies to thee Delete, Update methods of WorkitemController.

workitem.go, line 102 [r13] (raw file):

}

// Delete runs the delete action.

Can you elaborate on this please?

workitem.go, line 109 [r13] (raw file):

		return ctx.BadRequest(goa.ErrBadRequest(err.Error()))
	}
	tx := c.db.Begin()

See Create method for comment on defer.

workitem.go, line 125 [r13] (raw file):

}

// Update runs the update action.

Can you elaborate on this please?

workitem.go, line 134 [r13] (raw file):

	log.Printf("looking for id %d", idVal)
	tx := c.db.Begin()

See Create method for comment on defer.

workitem.go, line 140 [r13] (raw file):

		return ctx.NotFound()
	}
	if res.Version != ctx.Payload.Version {

Just a question: What's the concept here? Do we allow only exact version matches or maybe also allow for minor- or patch-level matching?

Comments from Reviewable

Reviewed 1 of 22 files at r9, 5 of 15 files at r11, 1 of 3 files at r12, 14 of 14 files at r13.
Review status: all files reviewed at latest revision, 17 unresolved discussions.

Comments from Reviewable

workitem.go, line 34 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> Maybe `could not parse ID of workitem` is a better error?!

Any reason why this isn't defined in the Design as "Param("id", Integer)" ?

Comments from Reviewable

Review status: all files reviewed at latest revision, 17 unresolved discussions.

workitem.go, line 29 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> Hm, can you elaborate on this please?

It's the standard comment generated by goa. This is just the implementation of the "show" action on the resource. What would you expect to see here?

workitem.go, line 34 [r13] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> Any reason why this isn't defined in the Design as "Param("id", Integer)" ?

Because I don't want clients to make any assumptions on id values, except that they are unique. Thus id values are strings.

workitem.go, line 43 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> This is just a comment on code style.
>
> > wiType := loadTypeFromDB(res.Type)
> > result, err := convertFromModel(*wiType, res)
>
> I think I read somewhere that it is good Go style to omit the first line and instead write
>
> ```go
> result, err := convertFromModel(*loadTypeFromDB(res.Type), res)
> ```
>
> instead.
>

I tend to not do that because it's easier to put a breaktpoint and inspect the value this way. Do they give any reasons why the inline function call should be better?

workitem.go, line 60 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> You can still write `:=` here and omit the `var err error`. `:=` also works if only part of the left hand side is undefined.

that doesn't work here: "non-name result.Fields[name] on left side of :=	workitem.go	/ALM Core	line 59	Go Problem"
But I guess it would work with variables?

workitem.go, line 81 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> You can still write `:=` here and omit the `var err error`. `:=` also works if only part of the left hand side is undefined.

See above...

workitem.go, line 86 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> I'm thinking for concurrency reasons we should maybe do it this way:
>
> ```go
> var err error
> tx := c.db.Begin()
> defer func() {
> 	if err != nil {
> 		tx.Rollback()
> 	}
> }()
> ```
>
> This way we just return in case of errors below and don't care about manual rollbacks. At least in my opinion this is safer and more elegant. What do you think? The only issue I see is that you may want to roll back without having `err` set to non `nil`. On example is this line in `Update` method:
>
> ```go
> if res.Version != ctx.Payload.Version {`
> ```
>
> You rollback in this if with `err` being `nil`.
>
> But still I like the concept of having a safety net with `defer`.
>
> Here's another idea where I substitute the `err` with a boolean that is always true unless we actually commit the `tx`:
>
> ```go
> rollback := true
> tx := c.db.Begin()
> defer func() {
> 	if rollback == true {
> 		tx.Rollback()
> 	}
> }()
>
> //...
>
> rollback = false
> tx.Commit()
> ```
>
> Now, that looks much more promising because the assumption is defensive and assumes an error rather than success.
>
> The same applies to thee `Delete`, `Update` methods of `WorkitemController`.

I don't think the first option buys us much for the reason you give. The second option is a bit safer when you have a single commit location in the func. On balance, I don't think either option saves us a lot of typing and it makes the control flow a bit less obvious...

workitem.go, line 102 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> Can you elaborate on this please?

see above

workitem.go, line 125 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> Can you elaborate on this please?

see above.

workitem.go, line 140 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> Just a question: What's the concept here? Do we allow only exact version matches or maybe also allow for minor- or patch-level matching?

Version here is the object version for optimistic concurrency control.

Comments from Reviewable

Review status: all files reviewed at latest revision, 16 unresolved discussions.

app/test/login_testing.go, line 1 [r11] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> app/test package should not be committed. Previously deleted in https://github.com/almighty/almighty-core/commit/47da9269b14b3fb35c9544ca543b0c7a103b070e and should be in .gitignore

it should be, but it isn't. Out of scope for this PR.

models/workitem.go, line 10 [r7] (raw file):
Previously, aslakknutsen (Aslak Knutsen) wrote…
> hehe, probably. :) Keep in mind that a WorkItem is more than Planner related. e.g. Not sure a Build has a Name? A Commit? A PullRequest Response? A Build Request? etc..

I still feel an user-level identifier for everyghing would be useful. I need to think about the concept more, though.

Comments from Reviewable

Review status: all files reviewed at latest revision, 12 unresolved discussions.

workitem.go, line 29 [r13] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> It's the standard comment generated by goa. This is just the implementation of the "show" action on the resource. What would you expect to see here?

It's just that a documentation that repeats the name of the function twice and only adds three boilerplate words is not good.

workitem.go, line 34 [r13] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> Because I don't want clients to make any assumptions on id values, except that they are unique. Thus id values are strings.

So Thomas, instead of saying what an ID's type is you let people guess? They will guess, believe me. And by letting them guess we introduce a possibility of misunderstanding.

workitem.go, line 43 [r13] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> I tend to not do that because it's easier to put a breaktpoint and inspect the value this way. Do they give any reasons why the inline function call should be better?

Good point! The breakpoints are important. I bet they advocated this due to the fact, that the wiType is not used anyhwere else.

workitem.go, line 86 [r13] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> I don't think the first option buys us much for the reason you give. The second option is a bit safer when you have a single commit location in the func. On balance, I don't think either option saves us a lot of typing and it makes the control flow a bit less obvious...

It's not about typing less but about having a safer way to rollback a tx. Using `defer` is common practice for closing sockets and file descriptors as well. Even if you have multiple commit locations you can still use my approach. And I'd say that it makes the control flow very obvious because when you read `c.db.Begin()` with the defer right below this line you know exactly that someone took care about "ending" it.

Comments from Reviewable

workitem.go, line 86 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> It's not about typing less but about having a safer way to rollback a tx. Using `defer` is common practice for closing sockets and file descriptors as well. Even if you have multiple commit locations you can still use my approach. And I'd say that it makes the control flow very obvious because when you read `c.db.Begin()` with the defer right below this line you know exactly that someone took care about "ending" it.

That is actually the whole idea behind defer. 'Show you close the resource where it was open' so you don't have to go hunting for it in the code somewhere and avoid 'missing' a control path.

Comments from Reviewable

workitem.go, line 29 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> It's just that a documentation that repeats the name of the function twice and only adds three boilerplate words is not good.

Just FYI, Goa only generates an example for this controller. After the first generation,  it's owned by us.

Comments from Reviewable

workitem.go, line 34 [r13] (raw file):
Previously, kwk (Konrad Kleine) wrote…
> So Thomas, instead of saying what an ID's type is you let people guess? They will guess, believe me. And by letting them guess we introduce a possibility of misunderstanding.

From a purist standpoint, the Rest resource shouldn't expose an ID.  The URL/href is the ID.  By exposing an ID you're implying/hinting that the client should be creating URL via string templates etc.

Comments from Reviewable

app/test/login_testing.go, line 1 [r11] (raw file):
Previously, tsmaeder (Thomas Mäder) wrote…
> it should be, but it isn't. Out of scope for this PR.

It's not out of scope to not commit stuff we have already deleted. But nm, I'll do it when merging.

Comments from Reviewable


Plugin Not Working

Trying to set up the plugin.
Confirmed the ip address is correct.
Does the plugin work with the Roku on Wifi or does it have to be wired ?

This is the error i get when pressing the button

Hi,
I am also unable to get it to work, although in my case there is no error message, I press the button and nothing happens!
I actually only need one function (turn off) since I use my TV as a monitor and have a SD action that shuts everything down for me at the end of the day.
I have triple-checked IP addresses and everything. I did notice that if you put an incorrect IP in, it still gets a check-mark, so I assume it doesn't verify the address is a valid Roku TV? Perhaps an enhancement?
Anyway, are there any setting on the TV I need to change to allow this to access? (FWIW, I also have the iOS app and that works fine.)
I'm out of ideas!
🤷♂

Sorry for the late response. Just got around to working on this plugin again. Are any of you still having this issue? If so, could you guys give some more info about your setups, such as:

Hardwired or WiFi on the PC or Phone App?
Hardwired or WiFi on the TV side?
What type of Roku TV is it? Built in or a set-top box?
Do you use an ISP provided router or your own? If so what router?

In my case, all hard wired. Built in Roku TV (TCL) My own Router - pfSense.

mine to started not working i have the correct ip, all my stuff is updated my pc is hardwired and my tv is built in roku on wirless network, it was working just before the new update with streamdeck that came out any help? ive used it before with no problems and have loved it really hope someway it can be helped thanks

I am also having a problem with the plugin not working. My MacBook Pro is hardwired and my Roku tv is on wifi like 3ft from the router provided by my ISP. I am able to still control the switch input/app function, but everything else has quit working. The TV IP address is correct in the Steamdeck plugin. Please help because I love using this plugin!

mine to started not working i have the correct ip, all my stuff is updated my pc is hardwired and my tv is built in roku on wirless network, it was working just before the new update with streamdeck that came out any help? ive used it before with no problems and have loved it really hope someway it can be helped thanks

Mine also just suddenly stopped working tonight. It's been working for at least a month since I got the Deck.
Onn Branded Roku Tv is wirless
computer is hardwired to a Netgear switch
Google ISP and google provided router

Another update: the launch app / select input feature still works,
but not up, down, left, right, back, select, home, vol+, vol-, mute

It's possible Roku has stopped supporting 3rd-party devices for remote control. https://developer.roku.com/docs/developer-program/dev-tools/external-control-api.md

For those that it doesn't work, could you guys try to run a curl command to the Roku TV and tell me what it returns.
On windows:

Open Command Prompt
Run "C:\curl http://replaceWithRokuTVIPaddress:8060/query/apps"

On Mac:

Open Terminal
Run "curl http://replaceWithRokuTVIPaddress:8060/query/apps"

For those that it doesn't work, could you guys try to run a curl command to the Roku TV and tell me what it returns.
On windows:

Open Command Prompt
Run "C:\curl http://replaceWithRokuTVIPaddress:8060/query/apps"

On Mac:

Open Terminal
Run "curl http://replaceWithRokuTVIPaddress:8060/query/apps"

curl: (28) Failed to connect to <IP_ADDRESS> port 8060 after 21031 ms: Couldn't connect to server

For those that it doesn't work, could you guys try to run a curl command to the Roku TV and tell me what it returns.
On windows:

Open Command Prompt
Run "C:\curl http://replaceWithRokuTVIPaddress:8060/query/apps"

On Mac:

Open Terminal
Run "curl http://replaceWithRokuTVIPaddress:8060/query/apps"

curl: (7) Failed to connect to (IP Address)  port 80 after 34 ms: Couldn't connect to server

all the sudden today, things started working again. So I ran the curl

HDMI 1 (ARC)
Live TV
HDMI 2
HDMI 3
AV
Netflix
Max
Prime Video
Hulu
Tubi - Free Movies & TV
YouTube
The Roku Channel
Peacock TV
Kids & Family on The Roku Channel
Disney Plus
discovery+ | Stream TV Shows
Pluto TV - It's Free TV
Apple TV
CBS
ESPN
Roku TV Intro
Kanopy
WatchFreeFlix
Fawesome - Free Movies and TV Shows
Sling TV - Live Sports, News, Shows + Freestream
Roku Photo Streams
Aquatic Life
Apple Music

Additionally, last night my deck went all wonky. Some icons were garbled, some icons were only taking up half the button, nothing at all was working, etc. I reinstalled streamdeck via the 'repair' option, and things still didn't work.
I literally unplugged the usb from the deck, and back in, and all my other commands started working
Then today, I noticed the TV controls work again. So, mayyybe my repair install + unplug and plug in had something to do with it. HOWEVER, Roku tv has had an update. It was previous version 12.5.5 build 4174, it's now version 12.5.5 build 4174-AT. So either the AT update they did fixed it, or my reinstall(repair) and unplugging and plugging back in helped.