feat (engine, ui, cli): broadcast #2574

yesnault · 2018-04-16T09:15:56Z

Description
Related issues
About tests
Mentions

@ovh/cds

Signed-off-by: Yvonnick Esnault [email protected]

yesnault · 2018-04-16T09:16:13Z

I'll add it on it.

fsamin · 2018-04-17T13:32:56Z

cli/cdsctl/admin_infos.go

there is no plural form of info:
https://www.linguee.com/english-french/translation/info.html

btw i don't think it's the right word, what's the goal ?

will be broadcast in next commit

bnjjj · 2018-04-17T14:27:12Z

engine/api/info/gorp_model.go

useless blank line

bnjjj · 2018-04-17T14:27:50Z

engine/api/info/info.go

bad comment I think

bnjjj · 2018-04-17T14:28:54Z

engine/api/info/info.go

no need of fmt.Sprintf

bnjjj · 2018-04-17T14:29:01Z

engine/api/info/info.go

useless sprintf

bnjjj · 2018-04-17T14:32:06Z

sdk/info.go

why not on cli ?

bnjjj · 2018-04-17T14:36:50Z

ui/src/app/views/admin/info/add/info.add.component.ts

can you use .pipe(finalize(() => this.loading = false)) please and delete duplicate this.loading = false;

bnjjj · 2018-04-17T14:38:45Z

ui/src/app/views/admin/info/add/info.add.html

switch to sui-select

bnjjj · 2018-04-17T14:39:59Z

ui/src/app/views/admin/info/edit/info.edit.component.ts

use .pipe(finalize... instead please

bnjjj · 2018-04-17T14:40:18Z

ui/src/app/views/admin/info/edit/info.edit.component.ts

.pipe(finalize

bnjjj · 2018-04-17T14:40:31Z

ui/src/app/views/admin/info/edit/info.edit.component.ts

.pipe(finalize

bnjjj · 2018-04-17T14:46:43Z

ui/src/app/views/admin/info/edit/info.edit.html

switch to sui-select

fsamin · 2018-04-23T06:29:52Z

cli/cdsctl/admin_broadcasts.go

+	Args: []cli.Arg{
+		{Name: "level"},
+		{Name: "title"},
+		{Name: "content"},


i could be nice to be able to user sdtin for the content
like cdsctl admin broadcast create info "Title" < changelog.md

fsamin · 2018-04-23T06:30:16Z

cli/cdsctl/admin_broadcasts.go

+	Name:  "create",
+	Short: "Create a CDS broadcast",
+	Args: []cli.Arg{
+		{Name: "level"},


i think level should be a flag with info as default value

fsamin · 2018-04-23T06:32:02Z

engine/api/broadcast.go

+		defer tx.Rollback()
+
+		// update broadcast in db
+		if err := broadcast.UpdateBroadcast(tx, bc); err != nil {


you should check the ID

fsamin · 2018-04-23T06:32:43Z

engine/api/broadcast.go

+
+func (api *API) getBroadcastsHandler() Handler {
+	return func(ctx context.Context, w http.ResponseWriter, r *http.Request) error {
+		if err := r.ParseForm(); err != nil {


is it usefull ?

there is no filter about level, date etc... ?

fsamin · 2018-04-23T06:33:50Z

engine/api/broadcast/broadcast.go

+}
+
+// UpdateBroadcast update a broadcast
+func UpdateBroadcast(db gorp.SqlExecutor, bc sdk.Broadcast) error {


for coherence with Insert, use a pointer

fsamin · 2018-04-23T06:34:47Z

engine/api/broadcast/gorp_model.go

+)
+
+// Broadcast is a gorp wrapper around sdk.Broadcast
+type Broadcast sdk.Broadcast


this type should be private

fsamin · 2018-04-23T06:35:40Z

engine/sql/090_broadcast.sql

+-- +migrate Up
+CREATE TABLE broadcast (
+  id BIGSERIAL PRIMARY KEY,
+  title TEXT NOT NULL default '',


VARCHAR 256

fsamin · 2018-04-23T06:36:12Z

engine/sql/090_broadcast.sql

+  id BIGSERIAL PRIMARY KEY,
+  title TEXT NOT NULL default '',
+  content TEXT NOT NULL default '',
+  level TEXT NOT NULL default '',


this is an enum, no need to have a TEXT field

fsamin · 2018-04-23T06:36:47Z

sdk/broadcast.go

+	Title    string    `json:"title" db:"title" cli:"title"`
+	Content  string    `json:"content" db:"content" cli:"content"`
+	Level    string    `json:"level" db:"level" cli:"level"`
+	Created  time.Time `json:"created" db:"created" cli:"-"`


isn't it interesting in the CLI ?

fsamin · 2018-04-23T06:38:02Z

sdk/cdsclient/interface.go


+// BroadcastClient expose all function for CDS Broadcasts
+type BroadcastClient interface {
+	Broadcasts() ([]sdk.Broadcast, error)


Broadcasts and BroadcastByLevel could be one function:
Broadcasts(filteredLevels ...string)

bnjjj · 2018-04-23T14:58:13Z

engine/api/broadcast/broadcast.go

+func LoadBroadcastByID(db gorp.SqlExecutor, id int64) (*sdk.Broadcast, error) {
+	dbBroadcast := broadcast{}
+	query := `select * from broadcast where id=$1`
+	args := []interface{}{id}


bnjjj · 2018-04-23T15:02:50Z

ui/src/assets/i18n/en.json

+  "broadcast_content": "Content",
+  "broadcast_archived": "Archived",
+  "broadcast_archived_help": "Check to archive this broadcast",
+  "broadcast_help_line_1": "",


why is it empty for two languages ?

bnjjj · 2018-04-23T15:03:18Z

ui/src/assets/i18n/fr.json

+  "broadcast_saved": "Information sauvegardée",
+  "broadcast_level": "Type",
+  "broadcast_none": "Aucune information",
+  "broadcast_load_broadcasts": "Loading broadcasts...",


Chargements des informations

fsamin

I think something that clean old broadcasts should be nice

fsamin · 2018-04-25T17:53:02Z

engine/api/broadcast/broadcast.go

if it's not too late:
InsertBroadcast should be Insert because broadcast.InsertBroadcast is annoying. broadcast.Insert is better

fsamin · 2018-04-25T17:53:09Z

engine/api/broadcast/broadcast.go

fsamin · 2018-04-25T17:53:22Z

engine/api/broadcast/broadcast.go

fsamin · 2018-04-25T17:53:35Z

engine/api/broadcast/broadcast.go

fsamin · 2018-04-25T17:53:43Z

engine/api/broadcast/broadcast.go

yesnault · 2018-04-25T22:08:27Z

clean old broadcasts: #2609 (to be discussed on how to clean)

sguiheux · 2018-04-30T15:33:36Z

ui/src/app/views/admin/broadcast/add/broadcast.add.component.ts

why subscribing to route params ?

move to constructor

sguiheux · 2018-04-30T15:35:17Z

ui/src/app/views/admin/broadcast/edit/broadcast.edit.component.ts

move to constructor?

sguiheux · 2018-04-30T15:36:47Z

ui/src/app/views/admin/broadcast/edit/broadcast.edit.component.ts

Cannot be equal to add , because you said that /add = AddComponent

sguiheux · 2018-04-30T15:37:56Z

ui/src/app/views/admin/broadcast/edit/broadcast.edit.component.ts

I prefer absolute to directly see where i am going

sguiheux · 2018-04-30T15:38:45Z

ui/src/app/views/admin/broadcast/edit/broadcast.edit.component.ts

move this to [disabled] on the button

sguiheux · 2018-04-30T15:39:32Z

ui/src/app/views/admin/broadcast/edit/broadcast.edit.component.ts

how is it possible, there is a add-broadcast component above

yesnault · 2018-05-01T18:23:56Z

@sguiheux / @bnjjj check sql file number before merging (it's 90 in this PR)

Signed-off-by: Yvonnick Esnault <[email protected]>

sguiheux · 2018-05-07T12:01:40Z

ui/src/app/views/admin/broadcast/add/broadcast.add.component.ts

    styleUrls: ['./broadcast.add.scss']
 })
-export class BroadcastAddComponent implements OnInit {
+export class BroadcastAddComponent {


AutoUnsubscribe

seen

yesnault requested review from bnjjj, fsamin and sguiheux April 16, 2018 09:15

yesnault added the review label Apr 16, 2018

fsamin requested changes Apr 17, 2018

View reviewed changes

bnjjj suggested changes Apr 17, 2018

View reviewed changes

ui/src/app/views/admin/info/edit/info.edit.html Outdated

Copy link

Contributor

bnjjj Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

switch to sui-select

sguiheux approved these changes Apr 17, 2018

View reviewed changes

yesnault changed the title ~~feat (engine, ui, cli): info~~ feat (engine, ui, cli): broadcast Apr 21, 2018

yesnault force-pushed the ye-global-info branch from b967077 to 2f6ede4 Compare April 22, 2018 08:50

yesnault added ready feature labels Apr 22, 2018

fsamin requested changes Apr 23, 2018

View reviewed changes

yesnault removed the ready label Apr 23, 2018

bnjjj suggested changes Apr 23, 2018

View reviewed changes

bnjjj approved these changes Apr 24, 2018

View reviewed changes

yesnault added the ready label Apr 24, 2018

fsamin previously requested changes Apr 25, 2018

View reviewed changes

sguiheux requested changes Apr 30, 2018

View reviewed changes

yesnault added ready and removed ready labels May 1, 2018

yesnault added 6 commits May 7, 2018 13:53

feat (engine, ui, cli): info

cf580ee

Signed-off-by: Yvonnick Esnault <[email protected]>

cr

ff75624

Signed-off-by: Yvonnick Esnault <[email protected]>

cdsctl admin broadcasts show|delete cmd

6d097cd

Signed-off-by: Yvonnick Esnault <[email protected]>

it

8684e74

Signed-off-by: Yvonnick Esnault <[email protected]>

remove logs

483a96d

Signed-off-by: Yvonnick Esnault <[email protected]>

cr

c320100

Signed-off-by: Yvonnick Esnault <[email protected]>

yesnault added 9 commits May 7, 2018 13:53

cr, a broadcast can be attached to one project

1a1b3c2

Signed-off-by: Yvonnick Esnault <[email protected]>

it broadcast

d540e96

Signed-off-by: Yvonnick Esnault <[email protected]>

it

6a9e172

Signed-off-by: Yvonnick Esnault <[email protected]>

fix it and now cascade (user have to remove broadcast if needed)

715b8eb

Signed-off-by: Yvonnick Esnault <[email protected]>

fix it - check it

5649169

Signed-off-by: Yvonnick Esnault <[email protected]>

cr

38ad737

Signed-off-by: Yvonnick Esnault <[email protected]>

cr

16f5bbc

Signed-off-by: Yvonnick Esnault <[email protected]>

cr navigate

f3e69f6

Signed-off-by: Yvonnick Esnault <[email protected]>

sql 91

7fb72ee

Signed-off-by: Yvonnick Esnault <[email protected]>

yesnault force-pushed the ye-global-info branch from ca624a0 to 7fb72ee Compare May 7, 2018 11:57

yesnault added 2 commits May 7, 2018 14:04

cr

0f2e24f

Signed-off-by: Yvonnick Esnault <[email protected]>

cr

0eb78ec

Signed-off-by: Yvonnick Esnault <[email protected]>

sguiheux approved these changes May 7, 2018

View reviewed changes

yesnault merged commit 1e15368 into master May 7, 2018

yesnault deleted the ye-global-info branch May 10, 2018 21:24

feat (engine, ui, cli): broadcast #2574

feat (engine, ui, cli): broadcast #2574

Uh oh!

Conversation

yesnault commented Apr 16, 2018

Uh oh!

yesnault commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsamin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yesnault commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fsamin left a comment •

edited

Loading