Added setting to remove admin link on status page #16
Conversation
addvanced
force-pushed
the
feature/optional-login-link
branch
from
a1429f6 to
b6aad4b
Compare
addvanced
force-pushed
the
feature/optional-login-link
branch
from
b6aad4b to
f599c87
Compare
| showAdminLink := r.PostFormValue("showAdminLink") | ||
|
|
||
| isShowAdminLinkEnabled := strings.ToLower(strings.TrimSpace(showAdminLink)) == "on" | ||
|
|
||
| tx, err := rwDB.Begin() | ||
| if err != nil { | ||
| log.Printf("postSettings.BeginShowAdminLink: %s", err) | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
| defer tx.Rollback() | ||
|
|
||
| err = updateMetaValue(tx, "showAdminLink", strconv.FormatBool(isShowAdminLinkEnabled)) | ||
| if err != nil { | ||
| log.Printf("postSettings.updateMetaValueShowAdminLink: %s", err) | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| if err := tx.Commit(); err != nil { | ||
| log.Printf("postSettings.CommitShowAdminLink: %s", err) | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| metaShowAdminLink = isShowAdminLinkEnabled |
There was a problem hiding this comment.
Currently this end up wiping out showAdminLink if you do any other action that hits postSettings.
Steps to reproduce:
- Check the manage this page option
- Rename your Statusnook instance
- Refresh the page, you'll see the box is unchecked
Could we only run these bits when showAdminLink is present to solve this?
| @@ -16056,6 +16110,32 @@ func getSettings(w http.ResponseWriter, r *http.Request) { | |||
| func postSettings(w http.ResponseWriter, r *http.Request) { | |||
| name := r.PostFormValue("name") | |||
| domain := strings.ToLower(r.PostFormValue("domain")) | |||
| showAdminLink := r.PostFormValue("showAdminLink") | |||
There was a problem hiding this comment.
I think all other form params are kebab-case, so I think we should go for show-admin-link.
On the other hand, meta.name values in the DB are camelCase 🙃
| showAdminLink, err := getMetaValue(tx, "showAdminLink") | ||
| if err != nil && !errors.Is(err, sql.ErrNoRows) { | ||
| log.Fatalf("main.getMetaValueShowAdminLink: %s", err) | ||
| return | ||
| } | ||
|
|
||
| metaShowAdminLink, err = strconv.ParseBool(showAdminLink) | ||
| if err != nil { | ||
| log.Fatalf("main.ParseBoolShowAdminLink: %s", err) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
We're going to need to handle the absence of showAdminLink in the DB, currently this would fail on ParseBool. I'm assuming you may have tested with a value manually entered into the DB, but let me know if I missed something.
Maybe we could do one of the following:
- Create a migration to add
showAdminLink - I think my preferred option would be - fallback to a default value when we see
sql.ErrNoRows(my vote would be true)
Let me know if I missed anything
There was a problem hiding this comment.
Are we going to add configuration for showAdminLink via the text-based config?
| showAdminLinkStr, err := getMetaValue(tx, "showAdminLink") | ||
| if err != nil && !errors.Is(err, sql.ErrNoRows) { | ||
| log.Printf("getSettings.getMetaValueShowAdminLink: %s", err) | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
|
|
||
| showAdminLinkEnabled := false | ||
| if showAdminLinkStr != "" { | ||
| showAdminLinkEnabled, err = strconv.ParseBool(showAdminLinkStr) | ||
| if err != nil { | ||
| log.Printf("getSettings.ParseBoolShowAdminLink: %s", err) | ||
| w.WriteHeader(http.StatusInternalServerError) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we drop all of this and use metaShowAdminLink, we're assigning it in main?
metaShowAdminLink onlys change in postSetttings and you're updating it after a successful transaction so it should always be consistent with the DB.
| {{if not .Ctx.Auth.ID}} | ||
| <a class="index-link index-link--secondary" href="/login" hx-boost="true">Manage this page</a> | ||
| {{if and .Ctx.ShowAdminLink (not .Ctx.Auth.ID)}} | ||
| <a class="index-link index-link--secondary" href="/login" hx-boost="true">Manage this page</a> |
There was a problem hiding this comment.
I think we've got a double indent here
| <a class="index-link index-link--secondary" href="/login" hx-boost="true">Manage this page</a> | |
| <a class="index-link index-link--secondary" href="/login" hx-boost="true">Manage this page</a> |
| @@ -827,6 +827,7 @@ var cancelAppCtx context.CancelFunc | |||
| var rwDB *sql.DB | |||
| var metaSetup string | |||
| var metaName string | |||
| var metaShowAdminLink bool | |||
There was a problem hiding this comment.
We probably don't need this globally, it's not as ubiquitous as something like metaName. I don't really mind right though now as there's not too many of these variables at the moment. I'll let you decide.
If you change this you should ignore my other feedback on dropping the reading & parsing in getSettings, it will be necessary.
| {{if .ShowAdminLink}} | ||
| checked | ||
| {{end}} | ||
| hx-trigger="change from:body" |
There was a problem hiding this comment.
I might be missing something, why do we need this hx-trigger?
There was a problem hiding this comment.
Oh I see. Is it to include the input value in request?
Maybe you wanted to say:
| hx-trigger="change from:body" | |
| hx-trigger="change from:#showAdminLinkSetting" |
There was a problem hiding this comment.
Let's keep things consistent and kebab-case show-admin-link-setting
| @@ -17325,6 +17406,7 @@ func getConfigSettings(w http.ResponseWriter, r *http.Request) { | |||
| GitHubToken: githubToken, | |||
| GitHubWebhookSecret: githubWebhookSecret, | |||
| Domain: metaDomain, | |||
| ShowAdminLink: metaShowAdminLink, | |||
| Ctx: getPageCtx(r), | |||
There was a problem hiding this comment.
I don't think we want this here, this is GET admin/settings/config-settings
| hx-trigger="change from:body" | ||
| hx-post="/admin/settings" | ||
| /> | ||
| Show 'Manage this page'-link on Status Page |
There was a problem hiding this comment.
Maybe "Show manage link on status page" would be neater
| <div class="checkbox-group"> | ||
| <label> | ||
| <input |
There was a problem hiding this comment.
Indentation is a bit off in this region
|
Thanks again for the PR. No plans to include something like this right now. |
No description provided.