Feature/visibility checks2 by jykae · Pull Request #1718 · apinf/platform

jykae · 2016-10-07T13:27:35Z

Closes #1624
Closes #1673
Closes #1686

Replaces #1697

jykae · 2016-10-07T13:39:09Z

@marla-singer this replaces #1697 could you review? As part of the process, when this gets merged, the old can be closed.

marla-singer · 2016-10-07T13:40:43Z

@jykae Check only the tags work or all visibility feature work?

jykae · 2016-10-07T13:42:41Z

@marla-singer my work is checks & tags, but take also overall view of the feature.

Have a nice weekend, I return to this on Monday, if anything.

marla-singer

Finished. Found some code style remarks and unexpected renaming css class

marla-singer · 2016-10-07T13:45:18Z

apis/client/view/header/header.html

          {{ api.name }}
          {{> viewApiStatus api=api width="0.4" }}
        </h1>
+        {{#unless api.isPublic }}


Please add a space between # and unless

marla-singer · 2016-10-07T13:47:15Z

catalogue/client/grid/grid.html

              {{ api.getApiManagersByName }}
            </span>
-          <p class="api-card-description">
+            {{#unless api.isPublic }}


Please add a space between # and unless

marla-singer · 2016-10-07T13:48:44Z

catalogue/client/table/table.html

-              {{_ "catalogueTable_added" }}
-              {{ api.relativeCreatedAt }}
-            </i>
+            {{#unless api.isPublic }}


Space between # and unless

marla-singer · 2016-10-07T13:54:32Z

catalogue/client/grid/grid.less

  margin-bottom: 1.87em;

-  .icon-indicator {
+  .api-status-color {


Why did you change it? Because of it the status indicator has wrong place:
Found:

Expected:

Or add class "pull-right" after variable "className" in this line
https://github.com/apinf/platform/blob/develop/apis/client/view/status/status.js#L23

I like the pull-right approach. Simple, and doesn't require any new CSS/Less.

@marla-singer pull-right fixes catalogue grid view but in API header:

I'd give styling for indicator outside status component. Because alignment might not always be the same. Or is it fine on top corner of text, and not vertical-center?

@jykae I checked on nightly. Yes, pull-right doesn't work as expected. So change style like

.api-status-color, .icon-indicator { ... }

marla-singer · 2016-10-07T14:05:43Z

@jykae Api owner doesn't watch the others public apis. Screenshots:

Go to catalog, pick the other public api.
Found result: Forbidden

Also it looks like you forgot add i18n tag to json file.

marla-singer · 2016-10-07T14:10:21Z

@jykae As for me the red rectangle part looks very strange. That's not obvious how i can use it?

jykae · 2016-10-10T08:23:56Z

@marla-singer Thanks for review, other things look fine founds, but the last one is not clear.

Authorized users part is @brylie 's work from another PR. Already accepted.
Only API owners & admins are able to view/edit API when it's private + authorized users can view
It is used to give single users permissions to view API even when API is globally marked Private.
@marla-singer Do you think it would need help text of some sort about usage? I think we can mark that as enhancement task.

marla-singer · 2016-10-10T08:34:49Z

@jykae Yes, I think It should be some message to describe the purpose of this field as sentence you wrote me above "It is used to give single users permissions to view API even when API is globally marked Private."
Okay, enhancement task sounds good too.

brylie · 2016-10-10T09:14:16Z

@jykae would you add the help text where @marla-singer suggests? Otherwise, how near is this PR to being merged?

jykae · 2016-10-10T11:11:02Z

@brylie @marla-singer Ok, I add suggested help text. Working on this now.

brylie · 2016-10-10T11:59:48Z

@jykae thanks. What is the estimate of time to complete?

jykae · 2016-10-10T12:05:26Z

@brylie 12.5 mins

brylie · 2016-10-10T12:29:51Z

Perfect!

jykae · 2016-10-10T12:58:44Z

@brylie sorry went bit over estimate, @marla-singer please review changes

marla-singer · 2016-10-10T13:00:47Z

core/lib/i18n/en.i18n.json

  "apiSettings_visibilityPanel_title": "Visibility",
  "apiSettings_visibility_heading": "Change API visibility",
  "apiSettings_visibility_text": "Make this API private or public",
+  "api_settings_visibility_authorizedUsers": "Give single users permissions to view API even when API is globally marked Private.",


Rename api_settings to apiSettings as above

@marla-singer done

marla-singer · 2016-10-10T13:23:04Z

@jykae api-indicator looks bad with class pull-right 😟

Please remove class 'pull-right' and return '.icon-indicator' class in grid.less
It's almost ready to merge. Just fix css style

Also these doesn't have any help text. Maybe you add like on screenshot? It's just text in <p> tag after <h3> tag

jykae · 2016-10-10T13:41:39Z

@marla-singer sorry, I don't know what you mean with "return '.icon-indicator' class in grid.less"

marla-singer · 2016-10-10T13:42:22Z

@jykae It must be so
https://github.com/apinf/platform/blob/develop/catalogue/client/grid/grid.less#L19

jykae · 2016-10-10T13:45:42Z

@marla-singer ok, done. I don't know how that dropped out.. :) Possibly because friday afternoon.

marla-singer · 2016-10-10T13:46:32Z

Merging

bajiat · 2016-10-10T13:47:24Z

thanks to both @jykae and @marla-singer

Ville Jyrkkä added 10 commits October 7, 2016 16:21

Header tag check, show quick tour for all

dafd385

Show forbidden if no permissions

1a6c496

Add forbidden template

c5d7e19

Update collection helpers

615d004

Tag for grid

c4fa3d6

Tag for table

8e2e008

Checks for catalogue publication, lint lint

adddb64

Add route for forbidden

9575c84

Move to error_templates

9fb7031

ProxyBackend info required for all users

d720009

jykae added this to the Sprint 32 milestone Oct 7, 2016

jykae added the in progress label Oct 7, 2016

Adjust monitoring status dot

3d312f0

jykae added the Ready for review label Oct 7, 2016

jykae mentioned this pull request Oct 7, 2016

New visibility feature #1697

Closed

jykae assigned marla-singer Oct 7, 2016

marla-singer reviewed Oct 7, 2016

View reviewed changes

marla-singer removed the Ready for review label Oct 7, 2016

Ville Jyrkkä added 3 commits October 10, 2016 14:34

update currentusercanview helper

eeb1810

Add space

47a5531

Add i18n

dc101bc

Ville Jyrkkä added 2 commits October 10, 2016 15:57

Add help text for authorized users

d595672

Align status indicator

8162c51

marla-singer reviewed Oct 10, 2016

View reviewed changes

Ville Jyrkkä added 2 commits October 10, 2016 16:02

Switch to camelCase

3e67c23

Fix to previous

fdf3a72

jykae added the Ready for review label Oct 10, 2016

Ville Jyrkkä added 2 commits October 10, 2016 16:40

show helptext as paragraph

1dd2bf9

Remove pull-right class

0dd5e87

Fix icon-indicator style

6d2b23b

marla-singer merged commit 3704e5c into develop Oct 10, 2016

marla-singer removed in progress Ready for review labels Oct 10, 2016

marla-singer deleted the feature/visibility-checks2 branch October 10, 2016 13:47

brylie modified the milestones: Sprint 33, Sprint 32 Oct 11, 2016

Conversation

jykae commented Oct 7, 2016

Uh oh!

jykae commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marla-singer commented Oct 7, 2016

Uh oh!

jykae commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marla-singer left a comment

Choose a reason for hiding this comment

Uh oh!

marla-singer Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

marla-singer Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

marla-singer Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

marla-singer Oct 7, 2016

Choose a reason for hiding this comment

Uh oh!

brylie Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

jykae Oct 10, 2016 • 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!

marla-singer Oct 10, 2016 • 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!

marla-singer commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marla-singer commented Oct 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jykae commented Oct 10, 2016

Uh oh!

marla-singer commented Oct 10, 2016

Uh oh!

brylie commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jykae commented Oct 10, 2016

Uh oh!

brylie commented Oct 10, 2016

Uh oh!

jykae commented Oct 10, 2016

Uh oh!

brylie commented Oct 10, 2016

Uh oh!

jykae commented Oct 10, 2016

Uh oh!

marla-singer Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

jykae Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

marla-singer commented Oct 10, 2016

Uh oh!

jykae commented Oct 10, 2016

Uh oh!

marla-singer commented Oct 10, 2016

Uh oh!

jykae commented Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marla-singer commented Oct 10, 2016

Uh oh!

bajiat commented Oct 10, 2016

Uh oh!

Reviewers

jykae commented Oct 7, 2016 •

edited

Loading

jykae commented Oct 7, 2016 •

edited

Loading

jykae Oct 10, 2016 •

edited

Loading

marla-singer Oct 10, 2016 •

edited

Loading

marla-singer commented Oct 7, 2016 •

edited

Loading

marla-singer commented Oct 7, 2016 •

edited

Loading

brylie commented Oct 10, 2016 •

edited

Loading

jykae commented Oct 10, 2016 •

edited

Loading