Skip to content

840 short description for each video#1510

Open
karjo24 wants to merge 30 commits intodevfrom
840-short-decription-for-each-video
Open

840 short description for each video#1510
karjo24 wants to merge 30 commits intodevfrom
840-short-decription-for-each-video

Conversation

@karjo24
Copy link
Copy Markdown
Contributor

@karjo24 karjo24 commented Mar 8, 2025

Motivation and Context

#840. Having the ability to make short notes or add/change the title of the lecture is useful for organising lectures when titles are not provided by the course admins.

Description

There's an additional option in the dropdown menu for each stream on the course overview page that allows users to input their own (limited to 80 characters) lecture title. There's a new API endpoint, that takes the new title and stores it in a new table in the database.

Steps for Testing

Prerequisites:

  • 1 User
  • 1 Course with 1 stream with title, 1 stream without
  1. Log in and navigate to course
  2. Click on the "More options" menu (three dots next to stream)
  3. Click on "Edit personal lecture title"
  4. Enter new title in popup menu
  5. Verify the following behaviour
    5.1 Pressing "Enter" or clicking outside the input field changes the title
    5.2 Pressing "Escape" disregards any changes
    5.3 Deleting the value of the input field resets the title to the actual title provided by the lecturer
  6. Reload to make sure the changes are persistent

Screenshots

Screencast.from.2025-03-08.15-19-45.mp4

@karjo24 karjo24 linked an issue Mar 8, 2025 that may be closed by this pull request
@karjo24 karjo24 self-assigned this Mar 8, 2025
@karjo24 karjo24 requested a review from a team March 8, 2025 15:07
Copy link
Copy Markdown
Member

@carlobortolan carlobortolan left a comment

Choose a reason for hiding this comment

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

Tested it locally and it looks good to me 👍

Just two more general questions:

  • Should the personal lecture title be displayed only in the course-view, or also in the "Recent VODs" and videoplayer/stream-view?
  • Should lecturers/admins of a course be allowed to add personal lecture titles, or would it be better to have the button set the actual lecture title to avoid confusion?

Comment thread api/courses.go Outdated
Comment thread dao/courses.go Outdated
Comment thread web/template/home.gohtml Outdated
@karjo24
Copy link
Copy Markdown
Contributor Author

karjo24 commented Mar 9, 2025

* Should the personal lecture title be displayed only in the course-view, or also in the "Recent VODs" and videoplayer/stream-view?

Yes, thanks for catching that. Somehow thought the title was not displayed there and didn't check. I'll change that.

* Should lecturers/admins of a course be allowed to add personal lecture titles, or would it be better to have the button set the actual lecture title to avoid confusion?

It probably shouldn't be left like this as I agree with the confusion part. I'm not sure, whether we should hide the menu option for admins or make it change the title globally. After all, that's what the admin page is for. I lean towards the "hide it"-approach, but I'm open for other opinions.

@carlobortolan
Copy link
Copy Markdown
Member

It probably shouldn't be left like this as I agree with the confusion part. I'm not sure, whether we should hide the menu option for admins or make it change the title globally. After all, that's what the admin page is for. I lean towards the "hide it"-approach, but I'm open for other opinions.

I think we can leave it out for now and add it in the future in a separate PR if necessary.

@carlobortolan carlobortolan added the enhancement New feature or request label Mar 11, 2025
Copy link
Copy Markdown
Member

@joschahenningsen joschahenningsen left a comment

Choose a reason for hiding this comment

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

Pretty nice! Left just a few comments

Comment thread api/courses.go Outdated
Comment thread dao/user_defined_lecture_titles.go
Comment thread dao/user_defined_lecture_titles.go Outdated
Comment on lines +47 to +50
// Upsert updates the entry if it exists, inserts it else
func (d userDefinedLectureTitlesDao) Upsert(userLectureTitle *model.UserDefinedLectureTitle) error {
return d.db.Save(userLectureTitle).Error
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need upserting? With user_id and stream_id being composite primary keys, we can use create with a conflict clause to update a record. See e.g.

err2 = DB.Clauses(clause.OnConflict{

I would go that way and rename the method to Save and drop Create and Upsert

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread model/stream.go Outdated
Comment on lines +404 to +406
if len(s.CustomLectureTitles) > 0 {
customLectureName = s.CustomLectureTitles[0].Title
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What exactly is happening here? We just use the first title? Wouldn't we send a random users title to someone?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Although this isn't set to a random users title, this should definitely be changed, yes, Idk what I was thinking. It wasn't set to a random title, because the CustomLectureTitles in this PR only get preloaded in conjunction with a Where clause, that checks the userId.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is now fixed. However, is there a cleaner way using Gorm to achieve the same to avoid having to add it manually afterwards?

// BeforeCreate is a GORM hook that is called before a new user is created.
// UserDefinedLectureTitle will not be saved if the title is too long
func (u *UserDefinedLectureTitle) BeforeCreate(tx *gorm.DB) (err error) {
if len(u.Title) > 256 {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you trim the whitespace? https://pkg.go.dev/strings#TrimSpace

Copy link
Copy Markdown
Contributor Author

@karjo24 karjo24 Mar 14, 2025

Choose a reason for hiding this comment

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

Is there any particular reason for this? Wouldn't this lead to a title being falsely accepted by BeforeCreate and then failing Save, if #whitespace + #meaningfulLetters > 256, but #meaningfulLetters < 256? Is this a best practice I'm not aware of?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Added strings.TrimSpace in both the model's BeforeCreate hook (model/lecture_titles.go) and in the API handler (api/stream.go) before the empty-check, so whitespace-only titles are correctly treated as a delete/reset. Commit: 681b37a.

Comment thread dao/courses.go Outdated
@karjo24 karjo24 mentioned this pull request Mar 14, 2025
@karjo24 karjo24 removed the request for review from joschahenningsen March 17, 2025 16:31
@SebiWrn
Copy link
Copy Markdown
Collaborator

SebiWrn commented Mar 18, 2025

Maybe there should also be a button for the user to delete the custom title, as just removing it to reset it to the lecturers title may not be intuitive.

@karjo24
Copy link
Copy Markdown
Contributor Author

karjo24 commented Mar 21, 2025

  • Should the personal lecture title be displayed only in the course-view, or also in the "Recent VODs" and videoplayer/stream-view?

Yes, thanks for catching that. Somehow thought the title was not displayed there and didn't check. I'll change that.

I'd postpone this change as this would require changing the signature of two methods, which are heavily used throughout the project, which would inflate this PR.

@SebiWrn SebiWrn changed the title 840 short decription for each video 840 short description for each video Apr 30, 2025
@joschahenningsen joschahenningsen self-requested a review May 9, 2025 11:15
@alexanderstephan
Copy link
Copy Markdown
Contributor

@copilot resolve the merge conflicts in this pull request.

Agent-Logs-Url: https://github.com/TUM-Dev/gocast/sessions/3e52aecf-8cbc-4bde-a8dc-d086283d8439

Co-authored-by: alexanderstephan <30267166+alexanderstephan@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 27, 2026

@copilot resolve the merge conflicts in this pull request.

Merge conflicts were already resolved in commit 56f1acd (Merge branch 'dev' into 840-short-decription-for-each-video). The branch is currently up to date.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Short Decription for each video

6 participants