Namespace selector #352

fabiovincenzi · 2023-09-20T07:56:17Z

fixes #202

dave-gantenbein · 2023-09-20T12:49:10Z

pkg/api/admin/dao/repositories/namespace.go

@@ -51,3 +53,12 @@ func (r NamespaceRepository) GetByCode(ctx context.Context, code string) (*model
 	}
 	return &namespace, nil
 }
+
+// ListNamespaces returns a list of all namespaces.
+func (r NamespaceRepository) ListNamespaces(ctx context.Context) ([]models.Namespace, error) {


What do we think of the stuttering implied by this naming convention, ie "namespaces" appearing twice: namespaceRepository.ListNamespaces. Maybe #List would be clear enough and shorter?

dsuhinin · 2023-09-20T12:49:23Z

pkg/ui/chooser/routes.go

+	}
+}
+
+func (r Router) AddRoutes(fr fiber.Router) {


can you please add comments for all public methods?

dsuhinin · 2023-09-20T12:51:23Z

pkg/ui/chooser/routes.go

+	}
+}
+
+func (r Router) AddRoutes(fr fiber.Router) {
 	sub, _ := fs.Sub(content, "embed")


I think we just skip error checking here, if so, can you please check it properly and return error to caller?

as I commented below, this cannot give an error as it just wouldn't build if there was no "embed" top directory

so I think we should keep skipping the error so we don't complicate the code further

sorry for the flip-flop @fabiovincenzi!

dsuhinin · 2023-09-20T13:19:11Z

pkg/api/admin/service/namespace/service.go

@@ -22,5 +22,9 @@ func NewService(
 }

 func (s Service) ListNamespaces(ctx context.Context) ([]models.Namespace, error) {


it is pretty simple now but we can create a unit tests here. what do you think?

yep, I'll do it

dsuhinin · 2023-09-20T13:19:56Z

pkg/ui/chooser/routes.go

 		Root: http.FS(sub),
 	}))
+	// engine and app for template rendering
+	engine := html.NewFileSystem(http.FS(sub), ".html")


can we put initialisation right into:

app := fiber.New(fiber.Config{ Views: html.NewFileSystem(http.FS(sub), ".html"), })

dsuhinin · 2023-09-20T13:53:26Z

pkg/api/admin/dao/repositories/namespace.go

@@ -16,6 +16,8 @@ type NamespaceRepositoryProvider interface {
 	Create(ctx context.Context, namespace *models.Namespace) error
 	// GetByCode returns namespace by its Code.
 	GetByCode(ctx context.Context, code string) (*models.Namespace, error)
+	// ListNamespaces returns all the namespaces
+	ListNamespaces(ctx context.Context) ([]models.Namespace, error)


yep, we can simplify it to be just List.

dsuhinin

Looks good for me. but will be nice to have other eyes on that. @jgiannuzzi

jgiannuzzi · 2023-09-26T09:36:16Z

pkg/api/admin/service/namespace/service.go

 func (s Service) ListNamespaces(ctx context.Context) ([]models.Namespace, error) {
-	return nil, nil
+	namespaces, err := s.namespaceRepository.List(ctx)


we should just return here

this is probably something old. maybe It seems like I left it after my tests. yep, please remove. fresh branches have no such change.

jgiannuzzi · 2023-09-26T09:40:23Z

pkg/ui/chooser/embed/index.html

+        <ul id="namespaces-list">
+            {{ range .Data }}
+            <li>
+                <a href="#" onclick="window.location = (window.location.origin + {{if eq .Code "default"}}'/'{{else}}'/ns/{{.Code}}/'{{end}});">{{.Code}}</a>    


the link text should be the namespace description, not its code

please make the onclick JS syntax consistent between all the links — we don't use parenthesis or semicolons above
it's fine to change it, but then change it everywhere

jgiannuzzi · 2023-09-26T09:42:25Z

pkg/ui/chooser/embed/index.html

+
+    <script>
+        function setCurrentNamespace() {
+            var currentUrl = window.location.href;
+            var namespaceIndex = currentUrl.indexOf('/ns/');
+            var namespaceText = document.getElementById('selected-namespace')
+            if (namespaceIndex !== -1) {
+                namespaceText.innerText = "Selected Namespace: " + currentUrl.substring(namespaceIndex + 4).slice(0, -1);
+            } else {
+                namespaceText.innerText = "Selected Namespace: default"
+            }
+        }
+
+        window.onload = setCurrentNamespace;
+    </script>


we know the selected namespace when this page gets rendered in the backend, please use that instead

How do we know the selected namespace from the backend?

by using namespace.GetNamespaceFromContext like we do in every controller

jgiannuzzi · 2023-09-26T09:43:58Z

pkg/ui/chooser/routes.go

+	sub, err := fs.Sub(content, "embed")
+	if err != nil {
+		return err
+	}


this cannot give an error as it just wouldn't build if there was no "embed" top directory

jgiannuzzi · 2023-09-26T10:08:28Z

pkg/ui/chooser/routes.go

+	// app for template rendering
+	app := fiber.New(fiber.Config{
+		Views: html.NewFileSystem(http.FS(sub), ".html"),
+	})
+	fr.Mount("/", app)
+
+	// specific routes
+	app.Get("/", r.controller.GetNamespaces)
+
+	// default route
+	app.Use("/", etag.New(), filesystem.New(filesystem.Config{
 		Root: http.FS(
 			common.NewOnlyRootFS(sub, "index.html"),
 		),
 	}))
+	return nil


there is an overlap between these various routes and the last one does not get used

…elector

jgiannuzzi · 2023-09-26T14:02:07Z

pkg/api/admin/dao/repositories/mock_namespace_repository_provider.go

please add this provider to .mockery.yaml

jgiannuzzi · 2023-09-26T14:05:36Z

pkg/ui/chooser/embed/index.html

+
+    <script>
+        function setCurrentNamespace() {
+            var currentUrl = window.location.href;
+            var namespaceIndex = currentUrl.indexOf('/ns/');
+            var namespaceText = document.getElementById('selected-namespace')
+            if (namespaceIndex !== -1) {
+                namespaceText.innerText = "Selected Namespace: " + currentUrl.substring(namespaceIndex + 4).slice(0, -1);
+            } else {
+                namespaceText.innerText = "Selected Namespace: default"
+            }
+        }
+
+        window.onload = setCurrentNamespace;
+    </script>


by using namespace.GetNamespaceFromContext like we do in every controller

jgiannuzzi · 2023-09-27T13:49:27Z

future improvement ideas:

show the active namespace in the top right corner
when this gets clicked, show the list of namespaces
(maybe) when there is only one namespace, don't display any namespace-related stuff

@dsuhinin

This PR is a collaborative effort between @dsuhinin, @fabiovincenzi, @suprjinx, and @jgiannuzzi. It incorporates the following PRs: * #340 * #391 * #397 * #352 * #402 * #421 * #420 * #395 * #454 * #275 --------- Co-authored-by: dsuhinin <7852635+dsuhinin@users.noreply.github.com> Co-authored-by: fabiovincenzi <93596376+fabiovincenzi@users.noreply.github.com> Co-authored-by: Geoffrey Wilson <geoff@gr-oss.io>

@dsuhinin

This PR is a collaborative effort between @dsuhinin, @fabiovincenzi, @suprjinx, and @jgiannuzzi. It incorporates the following PRs: * G-Research#340 * G-Research#391 * G-Research#397 * G-Research#352 * G-Research#402 * G-Research#421 * G-Research#420 * G-Research#395 * G-Research#454 * G-Research#275 --------- Co-authored-by: dsuhinin <7852635+dsuhinin@users.noreply.github.com> Co-authored-by: fabiovincenzi <93596376+fabiovincenzi@users.noreply.github.com> Co-authored-by: Geoffrey Wilson <geoff@gr-oss.io>

fabiovincenzi added 2 commits September 20, 2023 07:01

namespace selector

b1c7d29

go mod update

90c6125

fabiovincenzi linked an issue Sep 20, 2023 that may be closed by this pull request

Add namespace selection UI under /ns #202

Closed

2 tasks

fabiovincenzi added 2 commits September 20, 2023 08:01

removing unused variable

6dbfdc8

namespaces routes

3149f42

fabiovincenzi marked this pull request as ready for review September 20, 2023 11:38

fabiovincenzi added 2 commits September 20, 2023 11:40

removing comment

e86a97c

adding return in css

44ae3e6

dave-gantenbein reviewed Sep 20, 2023

View reviewed changes

dsuhinin requested changes Sep 20, 2023

View reviewed changes

fabiovincenzi added 2 commits September 21, 2023 08:31

add unit tests

c5dac93

generate mocks for admin repositories

dd599c8

fabiovincenzi requested review from dsuhinin and dave-gantenbein September 21, 2023 09:34

dsuhinin previously approved these changes Sep 21, 2023

View reviewed changes

dave-gantenbein previously approved these changes Sep 21, 2023

View reviewed changes

jgiannuzzi reviewed Sep 26, 2023

View reviewed changes

fabiovincenzi added 3 commits September 26, 2023 11:26

Merge remote-tracking branch 'upstream/features/namespaces' into ns-s…

f4b802d

…elector

merge namespaces branch changes

0f8895e

fix namespace service and links onclickC

5842bca

fabiovincenzi dismissed stale reviews from dave-gantenbein and dsuhinin via 5842bca September 26, 2023 12:20

jgiannuzzi reviewed Sep 26, 2023

View reviewed changes

fabiovincenzi added 3 commits September 26, 2023 17:50

fix mockery and use backand for current namespace

e0c5077

generate namespace mock

a5456f0

removing tab space

1009de6

jgiannuzzi approved these changes Sep 27, 2023

View reviewed changes

Merge branch 'features/namespaces' into ns-selector

dc7393f

jgiannuzzi merged commit 9ecd985 into G-Research:features/namespaces Sep 27, 2023

jgiannuzzi mentioned this pull request Oct 16, 2023

Initial support for namespaces #466

Merged

fabiovincenzi deleted the ns-selector branch November 2, 2023 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Namespace selector #352

Namespace selector #352

fabiovincenzi commented Sep 20, 2023

dave-gantenbein Sep 20, 2023

fabiovincenzi Sep 20, 2023

dsuhinin Sep 20, 2023

fabiovincenzi Sep 20, 2023

dsuhinin Sep 20, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

dsuhinin Sep 20, 2023

fabiovincenzi Sep 20, 2023

dsuhinin Sep 20, 2023

dsuhinin Sep 20, 2023

dsuhinin left a comment

jgiannuzzi Sep 26, 2023

dsuhinin Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

fabiovincenzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi Sep 26, 2023

jgiannuzzi commented Sep 27, 2023

		@@ -22,5 +22,9 @@ func NewService(
		}

		func (s Service) ListNamespaces(ctx context.Context) ([]models.Namespace, error) {

Namespace selector #352

Namespace selector #352

Conversation

fabiovincenzi commented Sep 20, 2023

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

dsuhinin 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

jgiannuzzi commented Sep 27, 2023