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.

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

fix #15 #25

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix #15 #25

wants to merge 3 commits into from

Conversation

frblo
Copy link
Collaborator

@frblo frblo commented Aug 18, 2024

Adds a page for displaying information about individual members of redaqtionen, along with all articles which they've been credited.

@frblo frblo requested review from Herkarl and foodelevator August 18, 2024 18:17
@frblo frblo requested review from foodelevator and removed request for foodelevator November 28, 2024 22:56
Copy link
Member

@foodelevator foodelevator left a comment

Choose a reason for hiding this comment

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

I read the *Ø***dbuggen and it was very nice


member, err := database.GetMember(db, memberID)
if err != nil {
c.Redirect(http.StatusNotFound, "")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't be a redirect if it's not a 3XX code (pretty sure gin will complain about this, but it's also against the web spec or whatever that thing is called)
Should probably be something c.Error or c.SetStatusCode or whatever (I have no idea what methods exist)

client/html/member.html Show resolved Hide resolved
if len(authors) == 1 {
return sb.String()
return template.HTML(sb.String())
Copy link
Member

Choose a reason for hiding this comment

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

You're only supposed to cast a string to a template.HTML if that is supposed to be interpreted as html, right? Are the author's names supposed to be interpreted as such?

}

func authorURL(name string, kthID string) template.HTML {
url := fmt.Sprintf(`<a href="/redaqtionen/%v">%v</a>`, kthID, name)
Copy link
Member

Choose a reason for hiding this comment

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

Same here as above. You should probably move some logic into the template rather than Sprintfing html here. Or this function could call a very small template.

@@ -485,7 +486,7 @@ func TestAuthortext(t *testing.T) {
// Test case 1: Valid AuthorText
authorText := sql.NullString{String: "Skriven av Test Testström", Valid: true}
authors := []database.Author{{PreferedName: sql.NullString{String: "Ej Korrektström", Valid: true}, KthID: "testsupp"}}
expected := "Skriven av Test Testström"
expected := template.HTML("Skriven av Test Testström")
Copy link
Member

@foodelevator foodelevator Nov 29, 2024

Choose a reason for hiding this comment

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

Is this what happens if you take soffan? (referring to the test, not this diff specifically)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants