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

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

Merged
merged 35 commits into from Oct 29, 2021
Merged

Conversation

mpro7
Copy link
Collaborator

@mpro7 mpro7 commented Oct 7, 2021

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
@@ -938,14 +789,14 @@ case class ListChildNodeADM(
val stringFormatter: StringFormatter = StringFormatter.getGeneralInstance

val unescapedLabels = stringFormatter.unescapeStringLiteralSeq(labels)
val unescapedComments = stringFormatter.unescapeStringLiteralSeq(comments)
val unescapedComments = stringFormatter.unescapeStringLiteralSeq(comments.get)
Copy link
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?

@@ -966,7 +817,7 @@ case class ListChildNodeADM(
* @return the comment in the preferred language.
*/
def getCommentInPreferredLanguage(userLang: String, fallbackLang: String): Option[String] =
comments.getPreferredLanguage(userLang, fallbackLang)
comments.get.getPreferredLanguage(userLang, fallbackLang)
Copy link
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?

@@ -1168,7 +1019,7 @@ trait ListADMJsonProtocol extends SprayJsonSupport with DefaultJsonProtocol with
"id" -> child.id.toJson,
"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
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?

@@ -12,7 +12,12 @@ object ListsMessagesUtilADM {
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
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
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
Collaborator 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
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
Collaborator 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.

@@ -90,7 +90,7 @@ case class ListGetResponseV2(list: ListADM, userLang: String, fallbackLang: Stri
val label: Map[IRI, JsonLDString] =
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
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
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
Collaborator 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
Copy link
Collaborator

subotic commented Oct 29, 2021

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

@mpro7
Copy link
Collaborator Author

mpro7 commented Oct 29, 2021

@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...

Copy link
Collaborator

@subotic subotic left a comment

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

@sonarcloud
Copy link

sonarcloud bot commented Oct 29, 2021

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.

None yet

2 participants