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

Add videos field to speaker #118

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

I-Mircheva
Copy link

Co-authored-by: AlexLaskin [email protected]
Co-authored-by: MagiMarinova [email protected]

I-Mircheva and others added 3 commits July 6, 2018 16:37
src/main/java/site/controller/AbstractCfpController.java Outdated Show resolved Hide resolved
@@ -141,6 +166,10 @@ public void deleteSpeaker(Long id){
}

public Speaker saveSpeaker(Speaker speaker){
if(speaker.getVideos() != null)
speaker.setVideos(videoSanitizerService.
Copy link
Contributor

Choose a reason for hiding this comment

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

missing { }

Copy link
Author

Choose a reason for hiding this comment

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

Fixed formatting and {}

src/main/java/site/controller/AbstractCfpController.java Outdated Show resolved Hide resolved
import org.springframework.stereotype.Service;

@Service
public class VideoSanitizerService {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like Util method for me. Also it doesn't work with a Video actually, it is more like working with a String name.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed: Not a service anymore, but a class StringSanitizer


private Boolean featured = false;

private Boolean accepted = false;

@Lob
private byte[] picture;

private String videos;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering why did you prefer to have them as comma separated values, instead of a List coming from a 1 to many relation?

Copy link
Author

Choose a reason for hiding this comment

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

  • We choose a String field as the forms when working with speaker would become too complicated
  • The admin panel for add/edit a speaker would be inconvenient

Copy link
Collaborator

Choose a reason for hiding this comment

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

@I-Mircheva can you make videos to be "videoLink" and to be just one URL mapped with

@column(length = 500)
private String videoLink;

for example so we dont need a migrate sql.

src/main/java/site/model/Speaker.java Outdated Show resolved Hide resolved
@@ -0,0 +1 @@
ALTER TABLE USER ADD COLUMN VIDEOS VARCHAR(255);
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, sometimes URLs are huge and with the comma separated approach 255 might not b enough in some cases

Copy link
Author

Choose a reason for hiding this comment

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

Made it VARCHAR(1024)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@I-Mircheva why do we need this SQL ? The schema should be updated automatically anyway.

Co-authored-by: AlexLaskin [email protected]
Co-authored-by: MagiMarinova [email protected]
@gochev gochev self-assigned this May 26, 2019
@gochev
Copy link
Collaborator

gochev commented May 26, 2019

Will merge it after jprime 2019 and before the new CFP.

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.

3 participants