Skip to content

feat: add value objects to list routes - old and new (DEV-65) - #1917

Merged
mpro7 merged 35 commits into
mainfrom
wip/DEV-65
Oct 29, 2021
Merged

mpro7 merged 35 commits into
mainfrom
wip/DEV-65

Conversation

@mpro7

@mpro7 mpro7 commented Oct 7, 2021

Copy link
Copy Markdown
Contributor

resolves DEV-65

@mpro7 mpro7 self-assigned this Oct 7, 2021
@mpro7
mpro7 marked this pull request as draft October 7, 2021 12:55
@mpro7
mpro7 marked this pull request as ready for review October 28, 2021 07:00
@mpro7 mpro7 changed the title wip/DEV-65 feat: add value objects to list routes - old and new (DEV-65) Oct 28, 2021
@mpro7
mpro7 requested a review from subotic October 28, 2021 07:14

val unescapedLabels = stringFormatter.unescapeStringLiteralSeq(labels)
val unescapedComments = stringFormatter.unescapeStringLiteralSeq(comments)
val unescapedComments = stringFormatter.unescapeStringLiteralSeq(comments.get)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What will happen if comments is None?

*/
def getCommentInPreferredLanguage(userLang: String, fallbackLang: String): Option[String] =
comments.getPreferredLanguage(userLang, fallbackLang)
comments.get.getPreferredLanguage(userLang, fallbackLang)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What will happen if comments is None?

"name" -> child.name.toJson,
"labels" -> JsArray(child.labels.stringLiterals.map(_.toJson)),
"comments" -> JsArray(child.comments.stringLiterals.map(_.toJson)),
"comments" -> JsArray(child.comments.get.stringLiterals.map(_.toJson)),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What will happen if comments is None?

val LIST_NODE_IRI_INVALID_ERROR = "List node IRI is invalid."
val PROJECT_IRI_MISSING_ERROR = "Project IRI cannot be empty."
val PROJECT_IRI_INVALID_ERROR = "Project IRI is invalid."
val LIST_NAME_MISSING_ERROR = "List name cannot be empty."

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A better name for ListsMessagesUtilADM would be ListsErrorMessagesADM

} else {
val validatedLabels = Try(value.map { label =>
val validatedLabel =
stringFormatter.toSparqlEncodedString(label.value, throw BadRequestException(LABEL_INVALID_ERROR))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that you need to wrap the stringFormatter call into a Try and not the whole sequence.

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.

Tried it out again and looks like I need to have the outer Try anyway, to perform final match.

} else {
val validatedComments = Try(value.map { comment =>
val validatedComment =
stringFormatter.toSparqlEncodedString(comment.value, throw BadRequestException(COMMENT_INVALID_ERROR))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that you need to wrap the stringFormatter call into a Try and not the whole sequence.

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.

I've tried the other way, but didn't work.

makeMapIriToJSONLDString(OntologyConstants.Rdfs.Label, node.labels, userLang, fallbackLang)
val comment: Map[IRI, JsonLDString] =
makeMapIriToJSONLDString(OntologyConstants.Rdfs.Comment, node.comments, userLang, fallbackLang)
makeMapIriToJSONLDString(OntologyConstants.Rdfs.Comment, node.comments.get, userLang, fallbackLang)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What happens if comments is None?

featureFactoryConfig: FeatureFactoryConfig
): Future[IRI] = {

// println("ZZZZZ-createNode", createNodeRequest)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

probably not needed

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.

Yes and not, left few commented out prints, just for a reference to other routes refactoring - will remove when finish that.

@subotic

subotic commented Oct 29, 2021

Copy link
Copy Markdown
Collaborator

@mpro7 you should also take a look at the code smells from from Sonar and get rid of those.

@mpro7

mpro7 commented Oct 29, 2021 •

Copy link
Copy Markdown
Contributor Author

@subotic most of the smells are TODOs which I added in places where I have either a question or somethings needs to be change in next tasks. Duplications are mainly caused by having 2 very similar route implementations - new and old. Could be removed if some parts of code would be moved to external helper methods. Not sure if we need it on this refactoring stage?

Another thing is that some code smells are ridiculous, like here https://sonarcloud.io/project/issues?id=dasch-swiss_dsp-api&open=AXy9QqxAZq5_O1fmTzTh&pullRequest=1917&resolved=false&types=CODE_SMELL

Not sure if this is my fault creating to generic messages or this test layout isn't the best to use for unit tests, because of so many cascades and repetitions...

@subotic subotic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks good. thanks :-)

id,
name,
labels,
comments = comments match {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

tip: you could use .getOrElse(StringLiteralSequenceV2(Vector.empty[StringLiteralV2])) instead of the match

@sonarqubecloud

Copy link
Copy Markdown

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 14 Code Smells

No Coverage information No Coverage information
13.5% 13.5% Duplication

@mpro7
mpro7 merged commit 7752a36 into main Oct 29, 2021
@mpro7
mpro7 deleted the wip/DEV-65 branch October 29, 2021 19:42
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