feat: Allow owner to disable guest links #646

x1uc · 2025-01-04T16:28:52Z

feat: Allow owner to disable guest links

This PR implements the feature request in #543.

To summarize what has been done:

Added a column (disabled) to the guest_links table & the GuestLink struct.
Updated the isActive check for guest links to include the disabled field.
Added a column on the guest links front page, with a button to enable/disable the link.
Added new APIs related to managing guest links' disabled state.

I’m a beginner, and I believe there may be many areas where my work lacks standardization from your perspective. Please point them out, and I will work on improving. I’m very glad to hear your suggestions, which will not only improve the quality of this project but also enhance my programming skills.

Screen

mtlynch

Great work! You did a good job of matching a lot of the conventions in the codebase so that your changes fit in elegantly.

I added some notes on places where I think we could improve, but I think this is off to a great start.

mtlynch · 2025-01-04T18:04:14Z

store/sqlite/guest_links.go

+func (s Store) DisableGuestLink(id picoshare.GuestLinkID) error {
+	log.Printf("Disable guest link %s", id)
+
+	tx, err := s.ctx.BeginTx(context.Background(), nil)


Transactions are useful when we have multiple SQL statements, and we want to rollback previous statements if any in the transaction fail.

When there's only one statement, we can just execute it immediately, like in InsertGuestLink.

(ditto below)

mtlynch · 2025-01-04T18:07:40Z

store/sqlite/guest_links.go

@@ -127,17 +129,62 @@ func (s Store) DeleteGuestLink(id picoshare.GuestLinkID) error {
 	return tx.Commit()
 }

+func (s Store) DisableGuestLink(id picoshare.GuestLinkID) error {
+	log.Printf("Disable guest link %s", id)


Log messages are not capitalized. Also, for concistency, it should be "disabling" so this should look like:

log.Printf("disabling guest link %s", id)

(ditto for the enable function)

handlers/guest_links.go

mtlynch · 2025-01-04T18:12:33Z

handlers/templates/pages/guest-link-index.html

@@ -125,6 +173,7 @@ <h1 class="title">Guest Links</h1>
          <th>Max Upload Size</th>
          <th>Uploads</th>
          <th class="has-text-right">Actions</th>
+          <th>Enable/Disable</th>


Can disable/enable be one of the actions rather than having its own column? I think the ban icon would work:

https://fontawesome.com/icons/ban?s=solid

I think I need an icon to represent 'revoke ban'. Which icon would be a good opposite of the ban icon? I’ve looked at a few, but none of them seem very appropriate.

I think the checkbox you're using now works well enough.

mtlynch · 2025-01-04T18:19:38Z

handlers/views.go

@@ -90,6 +90,9 @@ func (s Server) guestLinkIndexGet() http.HandlerFunc {
 		"isActive": func(gl picoshare.GuestLink) bool {
 			return gl.IsActive()
 		},
+		"isDisabled": func(gl picoshare.GuestLink) bool {


We don't need to do this, as the template can just call the struct's .IsDisabled function directly.

isActive set the wrong precedent, so I deleted that in #647.

mtlynch · 2025-01-04T18:38:20Z

handlers/routes.go

@@ -14,6 +14,8 @@ func (s *Server) routes() {
 	authenticatedApis.HandleFunc("/entry/{id}", s.entryDelete()).Methods(http.MethodDelete)
 	authenticatedApis.HandleFunc("/guest-links", s.guestLinksPost()).Methods(http.MethodPost)
 	authenticatedApis.HandleFunc("/guest-links/{id}", s.guestLinksDelete()).Methods(http.MethodDelete)
+	authenticatedApis.HandleFunc("/guest-links/enable/{id}", s.guestLinksEnable()).Methods(http.MethodPatch)
+	authenticatedApis.HandleFunc("/guest-links/disable/{id}", s.guestLinksDisable()).Methods(http.MethodPatch)


I think /guest-links/{id}/enable is a bit more idiomatic and consistent with our other routes.

Also, these seem like they should be PUT rather than PATCH as they're idempotent:

In comparison with PUT, a PATCH serves as a set of instructions for modifying a resource, whereas PUT represents a complete replacement of the resource. A PUT request is always idempotent (repeating the same request multiple times results in the resource remaining in the same state), whereas a PATCH request may not always be idempotent. For instance, if a resource includes an auto-incrementing counter, a PUT request will overwrite the counter (since it replaces the entire resource), but a PATCH request may not.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/PATCH

handlers/guest_links.go

mtlynch · 2025-01-04T18:40:40Z

handlers/templates/pages/guest-link-index.html

+          if (toggleBtn.getAttribute("aria-label") === "Enable") {
+            guestLinkEnable(id)
+              .then(() => {
+                const button = toggleBtn;


Why are we defining a new variable? Couldn't we just keep using toggleBtn?

mtlynch · 2025-01-04T18:41:53Z

handlers/templates/pages/guest-link-index.html

+                button.classList.remove("is-danger");
+                button.classList.add("is-success");
+                button.setAttribute("aria-label", "Disable");
+                button.innerHTML =


I try to avoid assigning .innerHTML since it's typically more powerful than we need. In this case, it looks like the only change is adding or removing fa-check-square. Can we do that with classList instead?

handlers/templates/pages/guest-link-index.html

x1uc · 2025-01-06T16:29:02Z

I apologize for the two unsuccessful pushes on CircleCI. Will Circle send you any email notifications about the CI failure and disturb you?
I noticed that the CONTRIBUTE.md mentions using npm run format to format the frontend code, but it doesn't seem to work. I think the script should be added to the package.json like this:

"scripts": {
  "format": "prettier --write ."
}

x1uc · 2025-01-06T16:34:20Z

picoshare/guest_link.go

@@ -55,12 +54,12 @@ func (gl GuestLink) IsExpired() bool {
 	return time.Now().After(time.Time(gl.UrlExpires))
 }

-func (gl GuestLink) IsDisabled() bool {
-	return gl.Disabled != GuestLinkDisabled && *gl.Disabled == 1
+func (gl GuestLink) IsDisabledLink() bool {


It seems that this function isn't actually needed, I only realized that after reviewing it later. I read your article on code reviews, and you mentioned that submitting code at night can sometimes cause trouble, and I think that's true.

mtlynch · 2025-01-06T18:31:21Z

I apologize for the two unsuccessful pushes on CircleCI. Will Circle send you any email notifications about the CI failure and disturb you?

No, it's fine. I don't receive any of those emails, so feel free to build as much as you'd like.

I noticed that the CONTRIBUTE.md mentions using npm run format to format the frontend code, but it doesn't seem to work.

Ah, yes, thanks for letting me know. I've fixed the instructions in #648.

x1uc · 2025-01-07T13:33:52Z

Changelist:

Rename the field from diabled to is_disabled .
Make disable/enable one of the actions.
Replace innerHtml manipulation with classList.toggle for cleaner DOM updates.
Refactor the API to follow RESTful conventions.
Implement end-to-end tests for this feature.
Add unit tests for guestLinksDisable and guestLinksEnable functions.

mtlynch · 2025-01-09T01:34:55Z

Thanks for updating! I started a review, but Github seems to be having an outage that's preventing me from adding more notes to the review. I'll try again tomorrow.

mtlynch

Great work on adding tests! You're continuing to match the conventions of the codebase really well and fit the feature to PicoShare's existing architecture.

I have a few smaller notes, but we're getting close to the finish line.

mtlynch · 2025-01-09T01:20:27Z

e2e/guest-links.spec.ts

+  expect(guestLinkRouteValue).not.toBeNull();
+  const guestLinkRoute = String(guestLinkRouteValue);
+
+  // Disable the guest link


(nit) Can we add a period at the end?

(ditto for all inline comments)

e2e/guest-links.spec.ts

handlers/guest_links_test.go

mtlynch · 2025-01-09T02:48:45Z

handlers/guest_links_test.go

+	s.Router().ServeHTTP(rec, req)
+	res := rec.Result()
+
+	if status := res.StatusCode; status != http.StatusNoContent {


Could we use the if got, want pattern here?

Sorry for the confusion. You did the right thing by following the existing conventions in the file, but I wrote the file before I learned the technique, so now I'm trying to write all new tests in the if got, want style.

This actually inspired me to write a blog post today about the pattern.

mtlynch · 2025-01-09T02:52:31Z

handlers/guest_links_test.go

+	}
+
+	if !gl.IsDisabled {
+		t.Fatalf("expected guest link to be disabled, got: %v", gl)


This can be t.Errorf. In general, we reserve t.Fatalf for cases where it's impossible for the test to continue. So, the previous instances make sense as t.Fatalf because there's no point in continuing if the status code is wrong or it can't retrieve the guest link, but just having the wrong state shouldn't really be fatal.

It's kind of moot because it's the last assertion in the test, but still.

mtlynch · 2025-01-09T02:53:39Z

handlers/guest_links_test.go

+	s.Router().ServeHTTP(rec, req)
+	res := rec.Result()
+
+	// File doesn't exist, but there's no error for enabling a non-existent file.


Shouldn't this fail? I feel like more sensible behavior would be for this to 404.

mtlynch · 2025-01-09T02:54:51Z

handlers/templates/pages/guest-link-index.html

+      // Switch the button color & icon
+      const buttonClasses = button.classList;
+      buttonClasses.toggle("is-success");
+      buttonClasses.toggle("is-danger");


I feel like the extra variable isn't worth it here. I think this way is actually clearer:

button.classList.toggle("is-success"); button.classList.toggle("is-danger");

(ditto for iconClasses)

mtlynch · 2025-01-09T02:59:11Z

handlers/templates/pages/guest-link-index.html

@@ -82,6 +86,60 @@
          });
      });
    });
+
+    const guestLinkButtonStates = {


Can we pull this into the event handler for toggleBtn.addEventListener("click"?

In general, I like variables to have as small a scope as possible to do what we need. Plus, it's helpful to have the declaration close to where we use it.

mtlynch · 2025-01-09T03:02:38Z

handlers/templates/pages/guest-link-index.html

+      },
+      Disable: {
+        apiCall: guestLinkDisable,
+        message: "Successfully disabled the guest link.",


Can we make this "Guest link disabled" to match the structure of the "enable" message?

mtlynch · 2025-01-09T03:02:49Z

handlers/templates/pages/guest-link-index.html

+    const guestLinkButtonStates = {
+      Enable: {
+        apiCall: guestLinkEnable,
+        message: "Guest link enabled successfully.",


I think we can cut out "successfully" for brevity.

x1uc · 2025-01-09T10:49:06Z

Change List

Refactored tests to use the if got, want style.
Refactored unit tests to adopt the table-driven test approach, reducing the number of test functions from 6 to 2.
Made several grammatical corrections.
Updated response status for invalid or non-existent guest_link id from 204 No Content to 404 Not Found .

mtlynch

Nice work! Just a little tidying up in the unit test, and then we should be ready to merge.

mtlynch · 2025-01-09T12:20:44Z

handlers/guest_links_test.go

+			}
+
+			// Copy the values that we can't predict in advance.
+			tt.expected.Created = gl.Created


This is an artifact of old tests we shouldn't repeat. I did this before I added the Clock interface, so I couldn't predict the Created time because it was just whatever time the test ran. But with the Clock interface, we control the creation time, so we don't have to mess with the time after.

I fixed this in the other test as #649.

mtlynch · 2025-01-09T13:35:56Z

handlers/guest_links_test.go

+			description: "disable an invalid guest link",
+			payload:     "i-am-an-invalid-link",
+			operation:   disableOperation,
+			expected:    http.StatusNotFound,


If it's an invalid guest link ID (i.e., contains illegal characters) that should be http.StatusBadRequest.

mtlynch · 2025-01-09T13:36:26Z

handlers/guest_links_test.go

+	disableOperation := createOperation("disable")
+	enableOperation := createOperation("enable")
+
+	//Function to create a new guest link.


Missing space after //. Should be // Function.

mtlynch · 2025-01-09T13:38:26Z

handlers/guest_links_test.go

+			tt.expected.Created = gl.Created
+			tt.expected.ID = picoshare.GuestLinkID(response.ID)
+
+			if !reflect.DeepEqual(gl, tt.expected) {


Can we switch this to if got, want? Like

if got, want := gl, tt.expected; !reflect.DeepEqual(got, want) {

Sorry for the bad examples elsewhere. I just fixed them in #650

mtlynch · 2025-01-09T13:48:48Z

handlers/guest_links_test.go

+
+func TestGuestLinkToggleInValidRequest(t *testing.T) {
+
+	type operation struct {


This is closer, but for both of these tests, we're adding a bit too much indirection and complexity in the test itself.

For test code, we want to minimize indirection so that we can verify it's correct by inspection. Google has a good blog post about this:

Testing on the Toilet: Don't Put Logic in Tests

We have to change the style in test code. In production code, it's more important to avoid duplicating logic, but in tests, I'm willing to sacrifice duplication if it makes the tests more obvious.

In table-driven tests, the tables should be plain data, with as little runtime processing as possible.

Like, compare the complexity of understanding a direct route like this /api/guest-links/abcdefgh23456789 as opposed to seeing fmt.Sprintf("/api/guest-links/%s/%s", id, action) and then tracing back to figure out what the value of id and action would be.

Yes, I read the article, and it makes sense. The issue in TestGuestLinkOperations can be easily avoided by modifying the code as follows:

[]struct { description string url string expected int }{ { description: "disable a nonexistent guest link", url: "/api/guest-links/abcdefgh23456789/disable", expected: http.StatusNotFound, }, }

However, the function TestGuestLinkToggleInvalidRequest also uses fmt.Sprintf("/api/guest-links/%s/%s", id, action) to construct URLs dynamically because the guest_link id is uncertain before the test runs. I'm not sure how to address this. Could you provide some advice on how to improve or refactor the function?

Additionally, I’m new to testing. In the codebase, I couldn’t find any indirect functions (like createLink or createOperation) being used in tests. Is it acceptable to use these indirect functions in tests, or is it better to avoid them for better test practices?

Yes, I read the article, and it makes sense. The issue in TestGuestLinkOperations can be easily avoided by modifying the code as follows:

Exactly! I think that version is much clearer.

However, the function TestGuestLinkToggleInvalidRequest also uses fmt.Sprintf("/api/guest-links/%s/%s", id, action) to construct URLs dynamically because the guest_link id is uncertain before the test runs. I'm not sure how to address this. Could you provide some advice on how to improve or refactor the function?

I think we should populate the datastore with a guest link we specify at the start of the test. That way, we know the ID. See TestGuestUpload as an example.

Additionally, I’m new to testing. In the codebase, I couldn’t find any indirect functions (like createLink or createOperation) being used in tests. Is it acceptable to use these indirect functions in tests, or is it better to avoid them for better test practices?

I think we should avoid them, as it forces the reader to jump around the file to understand the test logic. It's best if the reader can read the test from top to bottom without having to jump to previous function/variable definitions to understand what the test is doing.

…replace with class.toggle

…response values

x1uc · 2025-01-10T06:56:48Z

Change List:

Removed the createLink and createOperation methods.
Eliminate uncertain test values.
Replaced concatenated strings with direct URLs.
Changed HTTP response status for invalid guest_link id from 404 Not Found to 400 Bad Request.

mtlynch

Just a few small things on the test to wrap up.

I know this has been a long review, so if you'd prefer me to do the last bit of tidying, I can do that.

mtlynch · 2025-01-10T07:58:19Z

handlers/guest_links_test.go

@@ -346,3 +345,193 @@ func TestDeleteInvalidGuestLink(t *testing.T) {
 		t.Fatalf("DELETE returned wrong status code: got %v want %v", status, http.StatusBadRequest)
 	}
 }
+
+func TestGuestLinkToggleInValidRequest(t *testing.T) {
+	type testCase struct {


Can we define this inline like we do elsewhere?

picoshare/handlers/guest_links_test.go

Lines 21 to 27 in 37fb930

for _, tt := range []struct {

description string

payload string

currentTime time.Time

expected picoshare.GuestLink

status int

}{

mtlynch · 2025-01-10T07:59:35Z

handlers/guest_links_test.go

+			description: "guest_link operation: disable",
+			operationUrls: []string{
+				"/api/guest-links/abcdefgh23456789/disable",
+			},


Can we do a single API request in each testcase? I don't think we have any other tests where we call the endpoint in a loop.

I like the simplicity of:

Set up the server's state beforehand

Send a single request that changes the server's state

Test the server's state after that request

handlers/guest_links_test.go

mtlynch · 2025-01-10T08:32:45Z

handlers/guest_links_test.go

@@ -346,3 +345,193 @@ func TestDeleteInvalidGuestLink(t *testing.T) {
 		t.Fatalf("DELETE returned wrong status code: got %v want %v", status, http.StatusBadRequest)
 	}
 }
+
+func TestGuestLinkToggleInValidRequest(t *testing.T) {


The names seem to be flipped because these other test is called "TestGuestLinkOperations" but that's testing invalid input.

mtlynch · 2025-01-10T08:33:16Z

handlers/guest_links_test.go

+	}
+}
+
+func TestGuestLinkOperations(t *testing.T) {


Is it possible to merge this into the test above? I just did this with the creation tests:

picoshare/handlers/guest_links_test.go

Lines 20 to 275 in 37fb930

func TestGuestLinksPost(t *testing.T) {

for _, tt := range []struct {

description string

payload string

currentTime time.Time

expected picoshare.GuestLink

status int

}{

{

description: "minimally populated request",

payload: `{

"label": null,

"urlExpirationTime":"2030-01-02T03:04:25Z",

"fileLifetime":"876000h0m0s",

"maxFileBytes": null,

"maxFileUploads": null

}`,

currentTime: mustParseTime("2024-01-01T00:00:00Z"),

expected: picoshare.GuestLink{

Created: mustParseTime("2024-01-01T00:00:00Z"),

Label: picoshare.GuestLinkLabel(""),

UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),

FileLifetime: picoshare.FileLifetimeInfinite,

MaxFileBytes: picoshare.GuestUploadUnlimitedFileSize,

MaxFileUploads: picoshare.GuestUploadUnlimitedFileUploads,

},

status: http.StatusOK,

},

{

description: "fully populated request",

payload: `{

"label": "For my good pal, Maurice",

"urlExpirationTime":"2030-01-02T03:04:25Z",

"fileLifetime":"876000h0m0s",

"maxFileBytes": 1048576,

"maxFileUploads": 1

}`,

currentTime: mustParseTime("2024-01-01T00:00:00Z"),

expected: picoshare.GuestLink{

Created: mustParseTime("2024-01-01T00:00:00Z"),

Label: picoshare.GuestLinkLabel("For my good pal, Maurice"),

UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),

FileLifetime: picoshare.FileLifetimeInfinite,

MaxFileBytes: makeGuestUploadMaxFileBytes(1048576),

MaxFileUploads: makeGuestUploadCountLimit(1),

},

status: http.StatusOK,

},

{

description: "guest file expires in 1 day",

payload: `{

"label": "For my good pal, Maurice",

"urlExpirationTime":"2030-01-02T03:04:25Z",

"fileLifetime":"24h0m0s",

"maxFileBytes": 1048576,

"maxFileUploads": 1

}`,

currentTime: mustParseTime("2024-01-01T00:00:00Z"),

expected: picoshare.GuestLink{

Created: mustParseTime("2024-01-01T00:00:00Z"),

Label: picoshare.GuestLinkLabel("For my good pal, Maurice"),

UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),

FileLifetime: picoshare.NewFileLifetimeInDays(1),

MaxFileBytes: makeGuestUploadMaxFileBytes(1048576),

MaxFileUploads: makeGuestUploadCountLimit(1),

},

status: http.StatusOK,

},

{

description: "guest file expires in 30 days",

payload: `{

"label": "For my good pal, Maurice",

"urlExpirationTime":"2030-01-02T03:04:25Z",

"fileLifetime":"720h0m0s",

"maxFileBytes": 1048576,

"maxFileUploads": 1

}`,

currentTime: mustParseTime("2024-01-01T00:00:00Z"),

expected: picoshare.GuestLink{

Created: mustParseTime("2024-01-01T00:00:00Z"),

Label: picoshare.GuestLinkLabel("For my good pal, Maurice"),

UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),

FileLifetime: picoshare.NewFileLifetimeInDays(30),

MaxFileBytes: makeGuestUploadMaxFileBytes(1048576),

MaxFileUploads: makeGuestUploadCountLimit(1),

},

status: http.StatusOK,

},

{

description: "empty string",

payload: "",

status: http.StatusBadRequest,

},

{

description: "empty payload",

payload: "{}",

status: http.StatusBadRequest,

},

{

description: "invalid label field (non-string)",

payload: `{

"label": 5,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": null,

"maxFileUploads": null

}`,

status: http.StatusBadRequest,

},

{

description: "invalid label field (too long)",

payload: fmt.Sprintf(`{

"label": "%s",

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": null,

"maxFileUploads": null

}`, strings.Repeat("A", 201)),

status: http.StatusBadRequest,

},

{

description: "missing urlExpirationTime field",

payload: `{

"label": null,

"maxFileBytes": null,

"maxFileUploads": null

}`,

status: http.StatusBadRequest,

},

{

description: "invalid expirationTime field",

payload: `{

"label": null,

"urlExpirationTime": 25,

"maxFileBytes": null,

"maxFileUploads": null

}`,

status: http.StatusBadRequest,

},

{

description: "negative maxFileBytes field",

payload: `{

"label": null,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": -5,

"maxFileUploads": null

}`,

status: http.StatusBadRequest,

},

{

description: "decimal maxFileBytes field",

payload: `{

"label": null,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": 1.5,

"maxFileUploads": null

}`,

status: http.StatusBadRequest,

},

{

description: "too low a maxFileBytes field",

payload: `{

"label": null,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": 1,

"maxFileUploads": null

}`,

status: http.StatusBadRequest,

},

{

description: "zero maxFileBytes field",

payload: `{

"label": null,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": 0,

"maxFileUploads": null

}`,

status: http.StatusBadRequest,

},

{

description: "negative maxFileUploads field",

payload: `{

"label": null,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": null,

"maxFileUploads": -5

}`,

status: http.StatusBadRequest,

},

{

description: "decimal maxFileUploads field",

payload: `{

"label": null,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": null,

"maxFileUploads": 1.5

}`,

status: http.StatusBadRequest,

},

{

description: "zero maxFileUploads field",

payload: `{

"label": null,

"urlExpirationTime":"2025-01-01T00:00:00Z",

"maxFileBytes": null,

"maxFileUploads": 0

}`,

status: http.StatusBadRequest,

},

} {

t.Run(tt.description, func(t *testing.T) {

dataStore := test_sqlite.New()

c := mockClock{tt.currentTime}

s := handlers.New(mockAuthenticator{}, &dataStore, nilSpaceChecker, nilGarbageCollector, c)

req, err := http.NewRequest("POST", "/api/guest-links", strings.NewReader(tt.payload))

if err != nil {

t.Fatal(err)

}

req.Header.Add("Content-Type", "text/json")

rec := httptest.NewRecorder()

s.Router().ServeHTTP(rec, req)

res := rec.Result()

if got, want := res.StatusCode, tt.status; got != want {

t.Fatalf("status=%d, want=%d", got, want)

}

if tt.status != http.StatusOK {

return

}

body, err := io.ReadAll(res.Body)

if err != nil {

t.Fatal("failed to read response body")

}

var response handlers.GuestLinkPostResponse

err = json.Unmarshal(body, &response)

if err != nil {

t.Fatalf("response is not valid JSON: %v", body)

}

gl, err := dataStore.GetGuestLink(picoshare.GuestLinkID(response.ID))

if err != nil {

t.Fatalf("failed to retrieve guest link from datastore: %v", err)

}

// Copy the ID, which we can't predict in advance.

tt.expected.ID = picoshare.GuestLinkID(response.ID)

if got, want := gl, tt.expected; !reflect.DeepEqual(got, want) {

t.Fatalf("guestLink=%+v, want=%+v", got, want)

}

})

}

}

Do you mean merging this test into TestGuestLinksPost? Hmm, I’m not sure about that, as I believe TestGuestLinksPost is specifically for testing POST /api/guest-links. Could you offer some suggestions on how to approach this?

Sorry, no I'm saying merge together TestGuestLinkToggleInValidRequest and TestGuestLinkOperations

mtlynch · 2025-01-10T08:36:41Z

handlers/guest_links_test.go

+
+	for _, tt := range []testCase{
+		{
+			description: "guest_link operation: disable",


Can we make the descriptions be what we are saying the endpoint does. For example, for this one, I'd say, "disabling active guest link succeeds"

mtlynch · 2025-01-10T08:37:36Z

handlers/guest_links_test.go

+			},
+		},
+		{
+			description: "guest_link operation: enable",


I'd describe this one as "enabling guest link succeeds but has no effect when guest link is not disabled"

…o simplify guest link operations

x1uc · 2025-01-10T17:15:31Z

ChangeList:
0. Define struct like otherwise.

Add is_disabled field in test struct.
Do a single API request in each test case.
Refactor: unify two tests into a single one.
Change test description.

mtlynch

Looks good! Just a a small bit of tidying left.

mtlynch · 2025-01-11T01:21:28Z

handlers/guest_links_test.go

@@ -347,3 +346,164 @@ func TestDeleteInvalidGuestLink(t *testing.T) {
 		t.Fatalf("status=%d, want=%d", got, want)
 	}
 }
+
+func TestEnableDisableGuestLink(t *testing.T) {
+


(nit) Can we delete this blank line?

mtlynch · 2025-01-11T01:23:26Z

handlers/guest_links_test.go

+		t.Run(tt.description, func(t *testing.T) {
+			dataStore := test_sqlite.New()
+
+			if tt.status == http.StatusNoContent {


Can we change this to if !tt.guestLinkInStore.Empty() {?

I had trouble understand at first why we'd condition based on the HTTP response code when we hadn't even sent the HTTP request. I think checking based on whether the guest link is populated is the more direct way of testing whether we should add it into the mock store.

mtlynch · 2025-01-11T01:25:47Z

handlers/guest_links_test.go

+			res := rec.Result()
+
+			if got, want := res.StatusCode, tt.status; got != want {
+				t.Errorf("%s: handler returned wrong status code: got %v want %v",


This should be t.Fatalf. If the status code is wrong, there's no sense in checking the response data.

And can we simplify to this to match the other tests?

if got, want := res.StatusCode, tt.status; got != want { t.Fatalf("status=%d, want=%d", got, want) }

Including the description is an artifact from before I realized we could do t.Run.

mtlynch · 2025-01-11T01:26:37Z

handlers/guest_links_test.go

+			}
+
+			if got, want := gl, tt.expected; !reflect.DeepEqual(got, want) {
+				t.Fatalf("guestLink=%+v, want=%+v", got, want)


This should just be t.Errorf as it's not fatal for the result to not match expectations.

It's kind of moot since it's the last assertion in the test, but just good for consistency.

Makes sense. This might involve another error. I wrote this code to mimic this one:

picoshare/handlers/guest_links_test.go

Lines 270 to 272 in 37fb930

if got, want := gl, tt.expected; !reflect.DeepEqual(got, want) {

t.Fatalf("guestLink=%+v, want=%+v", got, want)

}

mtlynch · 2025-01-11T01:29:12Z

handlers/templates/pages/guest-link-index.html

+      const oldIcon = button.querySelector("i");
+      const iconClasses = oldIcon.classList;
+      iconClasses.toggle("fa-check-square");
+      iconClasses.toggle("fa-ban");


Can we make this

button.querySelector("i").toggle("fa-check-square"); button.querySelector("i").toggle("fa-ban");

I think I gave this note in a previous round but it might have been dropped.

Yes, I’m very sorry. You previously pointed out another issue, and I didn’t notice similar errors elsewhere. My apologies.

…icon toggling

mtlynch

Thanks so much! This looks good. I'm just going to go over it with some small tweaks, but I should be able to merge this in in the next few days.

mtlynch requested changes Jan 4, 2025

View reviewed changes

x1uc force-pushed the issue#543 branch 3 times, most recently from 288ffcb to e171ed5 Compare January 6, 2025 16:22

x1uc commented Jan 6, 2025

View reviewed changes

x1uc force-pushed the issue#543 branch from e171ed5 to 5e6e44d Compare January 7, 2025 06:51

x1uc requested a review from mtlynch January 7, 2025 12:44

mtlynch requested changes Jan 9, 2025

View reviewed changes

x1uc requested a review from mtlynch January 9, 2025 10:54

mtlynch requested changes Jan 9, 2025

View reviewed changes

x1uc added 15 commits January 10, 2025 14:31

Add disabled status field to guest_links table

bf0000a

Update GuestLink's IsActive method to consider disabled links

1aa841d

Add enable/disable button to guest_links

8f24912

Add enable/disable functionality with API and frontend integration

73c982b

fix: apply Prettier to fix code style issues

27d944c

refactor: rename Disabled field to IsDisabled and update related methods

cd89595

refactor: Merge disable/enable into Action, remove if-else loop, and …

c9f4405

…replace with class.toggle

refactor: Standardize API to RESTful style & fix logs & correct HTTP …

ef28fff

…response values

test: Add tests for enabling and disabling guest links

ee7aab7

test: add e2e test for guest link enable/disable functionality

58e7bf5

fix: apply Prettier to fix code style issues

36dfece

fix typos

a4f96e1

Refactor unit tests for guest_link

96630f4

refactor: streamline guest link button state management

6e74176

fix: apply Prettier to fix code style issues

1d8deba

x1uc added 2 commits January 10, 2025 14:31

chore: remove unused IDE configuration files

ae06b50

Reduce complexity in test code

a8d9cae

x1uc requested a review from mtlynch January 10, 2025 06:58

mtlynch requested changes Jan 10, 2025

View reviewed changes

refactor: unify two tests into a single one & add is_disabled field t…

a582c88

…o simplify guest link operations

x1uc force-pushed the issue#543 branch from 04baf68 to a582c88 Compare January 10, 2025 17:11

x1uc requested a review from mtlynch January 10, 2025 17:16

mtlynch requested changes Jan 11, 2025

View reviewed changes

refactor: improve test error handling and simplify guest link button …

bd55da4

…icon toggling

x1uc requested a review from mtlynch January 11, 2025 03:00

mtlynch added 9 commits January 11, 2025 07:32

Show aria-label in a tooltip

02fc76e

Set the guest link row to inactive on disable

c034a76

Fix snackbar message on guest link enable

e424f9f

Verify copy button disappears on guest link disable

2c85e76

Revise test descriptions

d2be9ce

Rename url to requestRoute

960da17

Make disable button before delete and make it a warning

12de2bc

Remove unused buttonClass property

074ce1b

Inline updateGuestLinkButtonStates

9a7a411

mtlynch approved these changes Jan 11, 2025

View reviewed changes

mtlynch added 4 commits January 11, 2025 09:50

Fix missing word

1292cd3

Delete unused iconClass property

aab4af0

Get rid of guest link button states

19c19c3

Combine enable and disable handlers

4987b4c

mtlynch merged commit 2e8c05f into mtlynch:master Jan 11, 2025
9 checks passed

github-actions bot locked and limited conversation to collaborators Jan 11, 2025


		func TestGuestLinkToggleInValidRequest(t *testing.T) {

		type operation struct {

	for _, tt := range []struct {
	description string
	payload string
	currentTime time.Time
	expected picoshare.GuestLink
	status int
	}{

	func TestGuestLinksPost(t *testing.T) {
	for _, tt := range []struct {
	description string
	payload string
	currentTime time.Time
	expected picoshare.GuestLink
	status int
	}{
	{
	description: "minimally populated request",
	payload: `{
	"label": null,
	"urlExpirationTime":"2030-01-02T03:04:25Z",
	"fileLifetime":"876000h0m0s",
	"maxFileBytes": null,
	"maxFileUploads": null
	}`,
	currentTime: mustParseTime("2024-01-01T00:00:00Z"),
	expected: picoshare.GuestLink{
	Created: mustParseTime("2024-01-01T00:00:00Z"),
	Label: picoshare.GuestLinkLabel(""),
	UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),
	FileLifetime: picoshare.FileLifetimeInfinite,
	MaxFileBytes: picoshare.GuestUploadUnlimitedFileSize,
	MaxFileUploads: picoshare.GuestUploadUnlimitedFileUploads,
	},
	status: http.StatusOK,
	},
	{
	description: "fully populated request",
	payload: `{
	"label": "For my good pal, Maurice",
	"urlExpirationTime":"2030-01-02T03:04:25Z",
	"fileLifetime":"876000h0m0s",
	"maxFileBytes": 1048576,
	"maxFileUploads": 1
	}`,
	currentTime: mustParseTime("2024-01-01T00:00:00Z"),
	expected: picoshare.GuestLink{
	Created: mustParseTime("2024-01-01T00:00:00Z"),
	Label: picoshare.GuestLinkLabel("For my good pal, Maurice"),
	UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),
	FileLifetime: picoshare.FileLifetimeInfinite,
	MaxFileBytes: makeGuestUploadMaxFileBytes(1048576),
	MaxFileUploads: makeGuestUploadCountLimit(1),
	},
	status: http.StatusOK,
	},
	{
	description: "guest file expires in 1 day",
	payload: `{
	"label": "For my good pal, Maurice",
	"urlExpirationTime":"2030-01-02T03:04:25Z",
	"fileLifetime":"24h0m0s",
	"maxFileBytes": 1048576,
	"maxFileUploads": 1
	}`,
	currentTime: mustParseTime("2024-01-01T00:00:00Z"),
	expected: picoshare.GuestLink{
	Created: mustParseTime("2024-01-01T00:00:00Z"),
	Label: picoshare.GuestLinkLabel("For my good pal, Maurice"),
	UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),
	FileLifetime: picoshare.NewFileLifetimeInDays(1),
	MaxFileBytes: makeGuestUploadMaxFileBytes(1048576),
	MaxFileUploads: makeGuestUploadCountLimit(1),
	},
	status: http.StatusOK,
	},
	{
	description: "guest file expires in 30 days",
	payload: `{
	"label": "For my good pal, Maurice",
	"urlExpirationTime":"2030-01-02T03:04:25Z",
	"fileLifetime":"720h0m0s",
	"maxFileBytes": 1048576,
	"maxFileUploads": 1
	}`,
	currentTime: mustParseTime("2024-01-01T00:00:00Z"),
	expected: picoshare.GuestLink{
	Created: mustParseTime("2024-01-01T00:00:00Z"),
	Label: picoshare.GuestLinkLabel("For my good pal, Maurice"),
	UrlExpires: mustParseExpirationTime("2030-01-02T03:04:25Z"),
	FileLifetime: picoshare.NewFileLifetimeInDays(30),
	MaxFileBytes: makeGuestUploadMaxFileBytes(1048576),
	MaxFileUploads: makeGuestUploadCountLimit(1),
	},
	status: http.StatusOK,
	},
	{
	description: "empty string",
	payload: "",
	status: http.StatusBadRequest,
	},
	{
	description: "empty payload",
	payload: "{}",
	status: http.StatusBadRequest,
	},
	{
	description: "invalid label field (non-string)",
	payload: `{
	"label": 5,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": null,
	"maxFileUploads": null
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "invalid label field (too long)",
	payload: fmt.Sprintf(`{
	"label": "%s",
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": null,
	"maxFileUploads": null
	}`, strings.Repeat("A", 201)),
	status: http.StatusBadRequest,
	},
	{
	description: "missing urlExpirationTime field",
	payload: `{
	"label": null,
	"maxFileBytes": null,
	"maxFileUploads": null
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "invalid expirationTime field",
	payload: `{
	"label": null,
	"urlExpirationTime": 25,
	"maxFileBytes": null,
	"maxFileUploads": null
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "negative maxFileBytes field",
	payload: `{
	"label": null,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": -5,
	"maxFileUploads": null
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "decimal maxFileBytes field",
	payload: `{
	"label": null,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": 1.5,
	"maxFileUploads": null
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "too low a maxFileBytes field",
	payload: `{
	"label": null,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": 1,
	"maxFileUploads": null
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "zero maxFileBytes field",
	payload: `{
	"label": null,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": 0,
	"maxFileUploads": null
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "negative maxFileUploads field",
	payload: `{
	"label": null,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": null,
	"maxFileUploads": -5
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "decimal maxFileUploads field",
	payload: `{
	"label": null,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": null,
	"maxFileUploads": 1.5
	}`,
	status: http.StatusBadRequest,
	},
	{
	description: "zero maxFileUploads field",
	payload: `{
	"label": null,
	"urlExpirationTime":"2025-01-01T00:00:00Z",
	"maxFileBytes": null,
	"maxFileUploads": 0
	}`,
	status: http.StatusBadRequest,
	},
	} {
	t.Run(tt.description, func(t *testing.T) {
	dataStore := test_sqlite.New()
	c := mockClock{tt.currentTime}
	s := handlers.New(mockAuthenticator{}, &dataStore, nilSpaceChecker, nilGarbageCollector, c)

	req, err := http.NewRequest("POST", "/api/guest-links", strings.NewReader(tt.payload))
	if err != nil {
	t.Fatal(err)
	}
	req.Header.Add("Content-Type", "text/json")

	rec := httptest.NewRecorder()
	s.Router().ServeHTTP(rec, req)
	res := rec.Result()

	if got, want := res.StatusCode, tt.status; got != want {
	t.Fatalf("status=%d, want=%d", got, want)
	}

	if tt.status != http.StatusOK {
	return
	}

	body, err := io.ReadAll(res.Body)
	if err != nil {
	t.Fatal("failed to read response body")
	}

	var response handlers.GuestLinkPostResponse
	err = json.Unmarshal(body, &response)
	if err != nil {
	t.Fatalf("response is not valid JSON: %v", body)
	}

	gl, err := dataStore.GetGuestLink(picoshare.GuestLinkID(response.ID))
	if err != nil {
	t.Fatalf("failed to retrieve guest link from datastore: %v", err)
	}

	// Copy the ID, which we can't predict in advance.
	tt.expected.ID = picoshare.GuestLinkID(response.ID)

	if got, want := gl, tt.expected; !reflect.DeepEqual(got, want) {
	t.Fatalf("guestLink=%+v, want=%+v", got, want)
	}
	})
	}
	}

feat: Allow owner to disable guest links #646

feat: Allow owner to disable guest links #646

Conversation

x1uc commented Jan 4, 2025

mtlynch 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

x1uc commented Jan 6, 2025

Choose a reason for hiding this comment

mtlynch commented Jan 6, 2025

x1uc commented Jan 7, 2025 • edited Loading

mtlynch commented Jan 9, 2025

mtlynch 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

x1uc commented Jan 9, 2025 • edited Loading

mtlynch 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

x1uc commented Jan 10, 2025

Change List:

mtlynch 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

x1uc commented Jan 10, 2025

mtlynch 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

mtlynch left a comment

Choose a reason for hiding this comment

x1uc commented Jan 7, 2025 •

edited

Loading

x1uc commented Jan 9, 2025 •

edited

Loading