Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added setting to remove admin link on status page #16

Closed
wants to merge 1 commit into from
Closed

Added setting to remove admin link on status page #16

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 87 additions & 5 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,7 @@ var cancelAppCtx context.CancelFunc
var rwDB *sql.DB
var metaSetup string
var metaName string
var metaShowAdminLink bool
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var metaDomain string
var metaUnconfirmedDomain string
var metaUnconfirmedDomainProblem string
Expand All @@ -845,6 +846,7 @@ type pageCtx struct {
HXRequest bool
HXBoosted bool
AdminArea bool
ShowAdminLink bool
Nav string
UnconfirmedDomainProblem string
UnconfirmedDomain string
Expand Down Expand Up @@ -879,6 +881,7 @@ func getPageCtx(r *http.Request) pageCtx {
HXRequest: r.Header.Get("HX-Request") == "true",
HXBoosted: r.Header.Get("HX-Boosted") == "true",
AdminArea: adminArea,
ShowAdminLink: metaShowAdminLink,
Nav: adminURLPrefix,
UnconfirmedDomainProblem: metaUnconfirmedDomainProblem,
UnconfirmedDomain: metaUnconfirmedDomain,
Expand Down Expand Up @@ -3553,6 +3556,18 @@ func main() {
}
metaName = name

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
}

Comment on lines +3559 to +3570
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we going to add configuration for showAdminLink via the text-based config?

domain, err := getMetaValue(tx, "domain")
if err != nil && !errors.Is(err, sql.ErrNoRows) {
log.Fatalf("main.getMetaValueDomain: %s", err)
Expand Down Expand Up @@ -5603,10 +5618,9 @@ func index(w http.ResponseWriter, r *http.Request) {


<a class="index-link" href="/history" hx-boost="true">View full history</a>
{{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>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we've got a double indent here

Suggested change
<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>

{{end}}

<dialog class="email-updates-modal">
<div>
<span>Get email updates</span>
Expand Down Expand Up @@ -15409,6 +15423,23 @@ func getSettings(w http.ResponseWriter, r *http.Request) {
return
}

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
}
}
Comment on lines +15426 to +15441
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: #16 (comment)


configFileEnabledStr, err := getMetaValue(tx, "configFileEnabled")
if err != nil && !errors.Is(err, sql.ErrNoRows) {
log.Printf("getSettings.getMetaValueConfigFileEnabled: %s", err)
Expand Down Expand Up @@ -15644,14 +15675,35 @@ func getSettings(w http.ResponseWriter, r *http.Request) {
<button
type="button"
class="cancel-button"
onclick="document.querySelector('#domain').disabled = true;"
>
onclick="document.querySelector('#domain').disabled = true;">
Comment on lines -15647 to +15678
Copy link
Owner

@goksan goksan Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like an unintentional diff

Cancel
</button>
</div>
{{end}}
</form>


<form id="showAdminLinkSetting" hx-post="" autocomplete="off" hx-swap="none">
<label for="general">
General
</label>
<div class="checkbox-group">
<label>
<input
Comment on lines +15690 to +15692
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation is a bit off in this region

name="showAdminLink"
type="checkbox"
{{if .ShowAdminLink}}
checked
{{end}}
hx-trigger="change from:body"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something, why do we need this hx-trigger?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Is it to include the input value in request?

Maybe you wanted to say:

Suggested change
hx-trigger="change from:body"
hx-trigger="change from:#showAdminLinkSetting"

Copy link
Owner

@goksan goksan Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep things consistent and kebab-case show-admin-link-setting

hx-post="/admin/settings"
/>
Show 'Manage this page'-link on Status Page
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Show manage link on status page" would be neater

</label>

</div>
</form>

<div class="settings-users-header">
<h3>Users</h3>
</div>
Expand Down Expand Up @@ -16029,6 +16081,7 @@ func getSettings(w http.ResponseWriter, r *http.Request) {
GitHubConfigSHA string
GitHubCommitLink string
GitHubConfigErrors []string
ShowAdminLink bool
Ctx pageCtx
}{
CurrentVersion: VERSION,
Expand All @@ -16043,6 +16096,7 @@ func getSettings(w http.ResponseWriter, r *http.Request) {
GitHubCommitLink: githubRepoURL + "/blob/" + githubConfigBranch + "/" +
githubConfigPath,
GitHubConfigErrors: githubConfigErrors,
ShowAdminLink: showAdminLinkEnabled,
Ctx: getPageCtx(r),
},
)
Expand All @@ -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")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙃


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
Comment on lines +16113 to +16138
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug.mp4


if name != "" {
if metaConfigFileEnabled {
Expand Down Expand Up @@ -17315,6 +17395,7 @@ func getConfigSettings(w http.ResponseWriter, r *http.Request) {
GitHubToken string
GitHubWebhookSecret string
Domain string
ShowAdminLink bool
Ctx pageCtx
}{
ConfigFile: configFile,
Expand All @@ -17325,6 +17406,7 @@ func getConfigSettings(w http.ResponseWriter, r *http.Request) {
GitHubToken: githubToken,
GitHubWebhookSecret: githubWebhookSecret,
Domain: metaDomain,
ShowAdminLink: metaShowAdminLink,
Ctx: getPageCtx(r),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want this here, this is GET admin/settings/config-settings

})
if err != nil {
Expand Down