Add visibility to component groups #2027

yoyosan · 2016-08-03T19:49:27Z

Hello.

The work on #1897 and #1892 is pretty much done so it's safe to say this PR is ready for code review.

The two tickets require that a component group have visibility for the public vs logged in users. Here's what I've done:

added two new columns to the component_groups table(check the last two migrations):
- visible, which specifies whether the component group can be visible to different kinds of users:
  - 0, viewable by public/guests
  - 1, only visible to logged in users,
  - 2, hidden to logged in users but visible to its creator. Default value
- created_by, which contains the user id of the component group creator
updated all related Command and CommandHandler classes and their tests
added functional tests for the StatusPageController and the DashboardController classes
added a couple of new methods to the AbstractTestCase class that take care of:
- setting up the project configuration before running the functional tests in order to avoid the setup installation page
- a sign in helper method that can be used to easily sign in users as needed for future tests

Also, I'd like to make have some discussions with you,on Slack/other IM if possible, regarding the following:

Should I move functional tests into their own directory e.g. tests/functional, integration tests into tests/integration, unit tests into tests/unit?
Is it ok with you if I remove the beUser methods from the AbstractApiTestCase and replace its usage with the newly added method, signIn, in the AbstractTestCase class?
The StatusPageControllerTest and DashboardControllerTest are a bit similar. I would move these methods in a trait which contains ComponentGroup creation methods?!

Thanks for reviewing this PR.

yoyosan · 2016-08-03T19:55:20Z

Right now, for the introduced test to work, you need to copy boostrap/cachet/production.php to boostrap/cachet/testing.php. I'm going to look for a better way though .

GrahamCampbell · 2016-08-04T20:15:41Z

tests/Http/Controllers/StatusPageControllerTest.php

@@ -138,7 +138,7 @@ protected function setupConfig()
            $settings = array_merge($this->app->config->get('setting'), $loaded);

            $this->app->config->set('setting', $settings);
-        } catch (Exception $e) {
+        } catch (\Exception $e) {


Please import this instead.

yoyosan · 2016-08-08T17:01:40Z

Hey @jbrooksuk and @GrahamCampbell.

Any news on this?

Thanks.

GrahamCampbell · 2016-08-08T17:17:51Z

If you could rebase without any merge commits, that would be great please.

yoyosan · 2016-08-08T17:48:59Z

Done.
I need to resolve the conflicts with 2.4.

LE: Hmm, I don't think I can though 😢

jbrooksuk · 2016-08-08T18:03:52Z

I need to resolve the conflicts with 2.4.

Switch to the 2.4 branch and run git fetch origin && git reset --hard origin/2.4.

GrahamCampbell · 2016-08-08T18:29:48Z

If there are conflicts, you're best off squashing first otherwise there could be more than one commit to resolve conflicts with.

yoyosan · 2016-08-09T05:05:50Z

Switch to the 2.4 branch and run git fetch origin && git reset --hard origin/2.4.

This doesn't work unless I get the latest changes from CachetHQ/Cachet:2.4 into my 2.4 branch, on yoyosan/Cachet.

I created a pull request for that and merged it into my fork 2.4 branch. Now I guess I should rebase add-visibility-to-component-groups with my fork's 2.4 branch?

yoyosan · 2016-08-09T18:30:28Z

@jbrooksuk and @GrahamCampbell, it should be good now.

GrahamCampbell · 2016-08-09T19:33:22Z

It would be cool to extend this to metrics too I think.

jbrooksuk · 2016-08-10T08:37:09Z

@jbrooksuk and @GrahamCampbell, it should be good now.

Thanks @yoyosan! We'll take a look at this shortly.

jbrooksuk · 2016-08-10T08:38:13Z

app/Bus/Commands/ComponentGroup/AddComponentGroupCommand.php

+     *
+     * @var int
+     */
+    public $visible;


I think this is better named as visibility or something. To me, visible implies a boolean.

Yes, makes sense. I'll do the modification.

Since we no longer have a third option, I think it should stay as it is now. This way it's identical to the Incident model. Don't you agree?

jbrooksuk · 2016-08-10T08:43:29Z

@yoyosan why are we concerned about which users own a component group?

yoyosan · 2016-09-13T12:27:38Z

@ConnorVG all done.

ConnorVG · 2016-09-13T12:42:03Z

app/Http/Controllers/Api/ComponentGroupController.php

@@ -29,14 +30,28 @@
 */
 class ComponentGroupController extends AbstractApiController
 {
+    protected $guard;


Docblock this please.

ConnorVG · 2016-09-13T12:49:10Z

Bar those last comments, I think this is pretty damn solid 😄

Thoughts 💢@GrahamCampbell💢?

ConnorVG

Are there any plans to add support to Components themselves? I'd imagine it'd make more sense (now that I think about it) if Components had visibility flags and when a ComponentGroup was queried it could check that (if none are available for the current visibility, it'd just not be accounted for, ie: has).

ConnorVG · 2016-09-15T08:57:38Z

app/Bus/Commands/ComponentGroup/AddComponentGroupCommand.php

@@ -40,14 +40,22 @@
    public $collapsed;

    /**
+     * To whom should the component group be visible?


This comment sort of makes it sound like you can make components visible to specific users which is confusing as that is not the case.

ConnorVG · 2016-09-15T08:58:15Z

app/Bus/Commands/ComponentGroup/UpdateComponentGroupCommand.php

@@ -49,14 +49,22 @@
    public $collapsed;

    /**
+     * To whom should the component group be visible?


ConnorVG · 2016-09-15T08:58:39Z

app/Bus/Handlers/Commands/ComponentGroup/AddComponentGroupCommandHandler.php

-            'collapsed' => $command->collapsed,
+            'name'       => $command->name,
+            'order'      => $command->order,
+            'collapsed'  => $command->collapsed,


One too many spaces before arrow.

ConnorVG · 2016-09-15T08:58:44Z

app/Bus/Handlers/Commands/ComponentGroup/UpdateComponentGroupCommandHandler.php

-            'collapsed' => $command->collapsed,
+            'name'       => $command->name,
+            'order'      => $command->order,
+            'collapsed'  => $command->collapsed,


One too many spaces before arrow.

ConnorVG · 2016-09-15T08:59:54Z

app/Composers/Modules/ComponentsComposer.php

+    protected $guard;
+
+    /**
+     * Creates a new Components composer instance.


Please strtolower the word Components

ConnorVG · 2016-09-15T09:05:52Z

app/Models/ComponentGroup.php

-        'order'     => 0,
-        'collapsed' => 0,
+        'order'      => 0,
+        'collapsed'  => 0,


Too many spaces again.

It's my curse 😿

ConnorVG · 2016-09-15T09:05:55Z

app/Models/ComponentGroup.php

-        'collapsed' => 'int',
+        'name'       => 'string',
+        'order'      => 'int',
+        'collapsed'  => 'int',


Too many spaces again.

ConnorVG · 2016-09-15T09:06:03Z

app/Models/ComponentGroup.php

-        'collapsed' => 'int',
+        'name'       => 'required|string',
+        'order'      => 'int',
+        'collapsed'  => 'int',


Too many spaces again.

ConnorVG · 2016-09-15T09:06:32Z

app/Models/User.php

+     *
+     * @return \Illuminate\Database\Eloquent\Relations\HasMany
+     */
+    public function componentGroups()


Is this needed?

Not really. 😊

But I think it makes sense to have the relation defined.

But a user can't 'own' a ComponentGroup?

Oh, you're right. It's related to the initial implementation.

I'll remove it.

ConnorVG · 2016-09-15T09:08:29Z

tests/Bus/Commands/ComponentGroup/UpdateComponentGroupCommandTest.php

+            'group'      => new ComponentGroup(),
+            'name'       => 'Foo',
+            'order'      => 1,
+            'collapsed'  => 2,


Too many spaces again.

yoyosan · 2016-09-17T08:18:05Z

Are there any plans to add support to Components themselves? I'd imagine it'd make more sense (now that I think about it) if Components had visibility flags and when a ComponentGroup was queried it could check that (if none are available for the current visibility, it'd just not be accounted for, ie: has).

Yes, I think it makes sense, but not in this PR. I can create another ticket for it.

yoyosan · 2016-09-17T08:24:35Z

Ping @ConnorVG!

yoyosan · 2016-09-19T11:35:15Z

Ping @GrahamCampbell and @jbrooksuk for feedback regarding this PR.

jbrooksuk · 2016-10-01T10:52:58Z

Hi @yoyosan can you rebase onto 2.4 please and then I'll merge 👍

Add functional test that asserts a guest can only see public items.

The missing `boostrap/cachet/testing.php` file is now generated the first time tests are ran.

Add constants for possible values for the visible column/field of the ComponentGroup model. Code review changes.

…1892. Add migration for the created_by column, in component_groups table. Add methods to the ComponentGroup and User models to be able to work with the created_by column. Hidden component groups are no longer displayed on the index page for loggedin users. Add functional test for the dashboard page. Save owner on create/edit component group. Update the API tests for Component group visibility feature.

Some code review fixes too.

… fixes and correct typo.

yoyosan · 2016-10-02T12:17:43Z

Hi @jbrooksuk! All done 🚀

jbrooksuk · 2016-10-02T12:58:04Z

Awesome, thanks! 👏🏻

jbrooksuk added this to the V2.4.0 milestone Aug 4, 2016

jbrooksuk added the Feature label Aug 4, 2016

GrahamCampbell reviewed Aug 4, 2016
View reviewed changes

yoyosan changed the title ~~[WIP] Add visibility to component groups~~ Add visibility to component groups Aug 7, 2016

jbrooksuk assigned jbrooksuk and GrahamCampbell Aug 10, 2016

jbrooksuk reviewed Aug 10, 2016
View reviewed changes

ConnorVG reviewed Sep 13, 2016
View reviewed changes

ConnorVG suggested changes Sep 15, 2016

View reviewed changes

Yoyosan and others added 15 commits October 2, 2016 15:16

Implement visibility for the components groups. Part of #1897.

21e0ac7

Add functional test that asserts a guest can only see public items.

Fix tests not running due to hitting the Setup page.

11b75e1

The missing `boostrap/cachet/testing.php` file is now generated the first time tests are ran.

Add a functional test that asserts logged in users can see all items.

11a3a70

Add constants for possible values for the visible column/field of the ComponentGroup model. Code review changes.

Add API tests for component group visibility feature.

8588806

Replace auth() usage with app(Guard::class).

1e502d8

Apply StyleCI fixes.

658c760

Drop the hidden visibility feature and fix all tests.

1b69813

Some code review fixes too.

Rename public to visible since it's a reserved keyword. Apply StyleCI…

1cfb26b

… fixes and correct typo.

Code review changes.

1cf11f1

Tidy up component and component groups gathering.

bc3cfea

Code review changes and StyleCI fixes.

8e3e401

Code review changes.

7de032b

Remove extra whitespace

09b4570

Remove useless method.

259672f

jbrooksuk merged commit ad0954e into cachethq:2.4 Oct 2, 2016

Add visibility to component groups #2027

Add visibility to component groups #2027

Conversation

yoyosan commented Aug 3, 2016 • edited Loading

yoyosan commented Aug 3, 2016 • edited Loading

Choose a reason for hiding this comment

yoyosan commented Aug 8, 2016

GrahamCampbell commented Aug 8, 2016

yoyosan commented Aug 8, 2016 • edited Loading

jbrooksuk commented Aug 8, 2016

GrahamCampbell commented Aug 8, 2016

yoyosan commented Aug 9, 2016 • edited Loading

yoyosan commented Aug 9, 2016 • edited Loading

GrahamCampbell commented Aug 9, 2016

jbrooksuk commented Aug 10, 2016

Choose a reason for hiding this comment

yoyosan Aug 10, 2016 • edited Loading

Choose a reason for hiding this comment

yoyosan Aug 11, 2016 • edited Loading

Choose a reason for hiding this comment

jbrooksuk commented Aug 10, 2016

yoyosan commented Sep 13, 2016

Choose a reason for hiding this comment

ConnorVG commented Sep 13, 2016

ConnorVG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoyosan Sep 17, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoyosan Sep 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoyosan commented Sep 17, 2016

yoyosan commented Sep 17, 2016

yoyosan commented Sep 19, 2016

jbrooksuk commented Oct 1, 2016

yoyosan commented Oct 2, 2016

jbrooksuk commented Oct 2, 2016

yoyosan commented Aug 3, 2016 •

edited

Loading

yoyosan commented Aug 3, 2016 •

edited

Loading

yoyosan commented Aug 8, 2016 •

edited

Loading

yoyosan commented Aug 9, 2016 •

edited

Loading

yoyosan commented Aug 9, 2016 •

edited

Loading

yoyosan Aug 10, 2016 •

edited

Loading

yoyosan Aug 11, 2016 •

edited

Loading

yoyosan Sep 17, 2016 •

edited

Loading

yoyosan Sep 19, 2016 •

edited

Loading