-
Notifications
You must be signed in to change notification settings - Fork 3
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
Story/vspc 196 #344
base: develop
Are you sure you want to change the base?
Story/vspc 196 #344
Conversation
Can one of the admins verify this patch? |
please reopen existing pr so the comment history is kept |
Resolve conflicts please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll stop here. There seem to be a bunch of wrong formatting and indentation changes, and in general unnecessary changes. Please fix first before I review.
vspace/pom.xml
Outdated
<javers.version>5.15.0</javers.version> | ||
<spring-security-version>5.3.3.RELEASE</spring-security-version> | ||
<thymeleaf.version>3.0.11.RELEASE</thymeleaf.version> | ||
<javers.version>5.11.1</javers.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there are reason these are being downgraded?
vspace/pom.xml
Outdated
<db.url>jdbc:mysql://localhost:3306/vspace?serverTimezone=UTC</db.url> | ||
<db.user>root</db.user> | ||
<db.password>Metroplaza@2023</db.password> | ||
<uploaded.files.path>/Users/ajayyadav/Downloads/Vpsace_images</uploaded.files.path> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not be changed
vspace/pom.xml
Outdated
<groupId>javax.xml.bind</groupId> | ||
<artifactId>jaxb-api</artifactId> | ||
<version>2.3.1</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting
vspace/pom.xml
Outdated
<artifactId>thymeleaf-extras-springsecurity5</artifactId> | ||
<version>3.0.3.RELEASE</version> | ||
</dependency> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatting
vspace/pom.xml
Outdated
@@ -424,4 +406,5 @@ | |||
</plugin> | |||
</plugins> | |||
</build> | |||
</project> | |||
|
|||
</project> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are a bunch of versions being downgraded here which they shouldn't
vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExternalLinkManager.java
Show resolved
Hide resolved
protected abstract T getTarget(String linkedId); | ||
|
||
protected abstract U createDisplayLink(L link); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm so confused, why is this here? I this needed for this ticket?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah I see, this should not be moved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came from merging changes from the main branch
@RequestParam("moduleLinkLabel") String moduleLinkLabel, @RequestParam("moduleType") String displayType, | ||
@RequestParam("moduleLinkImage") MultipartFile file) | ||
throws NumberFormatException, SpaceDoesNotExistException, IOException, ImageCouldNotBeStoredException { | ||
public ResponseEntity<String> createModuleLink(@PathVariable("id") String id, @RequestParam("x") String x, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is wrong I believe
This reverts commit fda3666. revert
This reverts commit 8f67f83. changeing it back
protected abstract T getTarget(String linkedId); | ||
|
||
protected abstract U createDisplayLink(L link); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
@@ -119,7 +119,7 @@ | |||
{ "code": "na", "label": "Nauru" }, | |||
{ | |||
"code": "nb", | |||
"label": "Bokm�l, Norwegian; Norwegian Bokm�l" | |||
"label": "BokmÂl, Norwegian; Norwegian BokmÂl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this being changed?
@@ -184,7 +184,7 @@ | |||
{ "code": "uz", "label": "Uzbek" }, | |||
{ "code": "ve", "label": "Venda" }, | |||
{ "code": "vi", "label": "Vietlabelse" }, | |||
{ "code": "vo", "label": "Volap�k" }, | |||
{ "code": "vo", "label": "Volap¸k" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or this?
* @param search This is the search term used to find spaces by name. | ||
* @return A ResponseEntity containing a JSON array of spaces (ID and name) and HTTP status OK. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually what is this javaodc for?
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too many empty lines
display.getPositionY(), display.getRotation(), display.getExternalLink().getExternalLink(), title, | ||
displayType, null, null); | ||
display.getPositionY(), display.getRotation(), display.getExternalLink().getExternalLink(), title, | ||
displayType, null, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a lot of wrong indentation in this file
…aces-2.0 into story/vspc-196 merge changes
there are some code factor issues but I believe they are not currently affecting the app's functionality. |
Make it so, Jenkins. |
there are test failures. |
Guidelines for Pull Requests
If you haven't yet read our code review guidelines, please do so, You can find them here.
Please confirm the following by adding an x for each item (turn
[ ]
into[x]
).Please provide a brief description of your ticket
(you can copy the ticket if it hasn't changed)
It should be possible to reuse link images
Anything else the reviewer needs to know?
https://diging.atlassian.net/browse/VSPC-196?focusedCommentId=24055