Story/vspc 213 #302

pkharge · 2022-05-23T23:18:51Z

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]).

I have removed all code style changes that are not necessary (e.g. changing blanks across the whole file that don’t need to be changed, adding empty lines in parts other than your own code)
I am not making any changes to files that don’t have any effect (e.g. imports added that don’t need to be added)
I do not have any sysout statements in my code or commented out code that isn’t needed anymore
I am not reformatting any files in the wrong format or without cause.
I am not changing file encoding or line endings to something else than UTF-8, LF
My pull request does not show an insane amount of files being changed although my ticket only requires a few files being changed
I have added Javadoc/documentation where appropriate
I have added test cases where appropriate
I have explained any part of my code/implementation decisions that is not be self-explanatory

Please provide a brief description of your ticket

(you can copy the ticket if it hasn't changed)

In the exhibition configuration, users should be able to specify multiple languages for the exhibition. This ticket is just for the specification of languages, not providing text for multiple languages. So, all this needs to be doing is letting the user select languages from a list of languages. There needs to be a default language as well. So the user would select one of the languages as default language.

The follow up tickets will deal with adding slide and space texts in multiple languages. So the data model for this ticket should be a new Language or ExhibitionLanguage class which at the moment won’t have much additional info besides a label and a 2 character ISO code (e.g. en, de, it,…).

Anything else the reviewer needs to know?

... describe here ...

diging-jenkins · 2022-05-23T23:18:53Z

Can one of the admins verify this patch?

jdamerow · 2022-05-25T20:22:33Z

vspace/src/main/java/edu/asu/diging/vspace/config/ExhibitionLanguageConfig.java

+    @Autowired
+    private Environment environment;
+
+    List<Map>  exhibitionLanguageList = new ArrayList<Map>();


shoudl be private

jdamerow · 2022-05-25T20:25:48Z

vspace/src/main/java/edu/asu/diging/vspace/config/ExhibitionLanguageConfig.java

+     */
+    @PostConstruct
+    public void init() {
+        for(Iterator it = ((AbstractEnvironment) environment).getPropertySources().iterator(); it.hasNext(); ) {


since only one is listed in the PropertySource, can there be more in the list than just this one?

Didn't get you. There are multiple property sources. we are iterating through it to search for the particular property we need

Are there more than the one listed in the @PropertySource annoation?

Yes, there are around 10 in the list.

Can't you use get here: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/core/env/MutablePropertySources.html#get-java.lang.String-

jdamerow · 2022-05-25T20:26:17Z

vspace/src/main/java/edu/asu/diging/vspace/config/ExhibitionLanguageConfig.java

+            org.springframework.core.env.PropertySource propertySource = (org.springframework.core.env.PropertySource) it.next();
+            if (propertySource instanceof MapPropertySource) {
+                MapPropertySource mapSource = ((MapPropertySource) propertySource);
+                if("json-property".equals(mapSource.getName())) {


strings should never be hard-coded. If a string is needed here, it should be in a constant.

jdamerow · 2022-05-25T20:27:46Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/ExhibitionLanguage.java

+
+    public ExhibitionLanguage() {
+        super();
+    }


is this constructor necessary?

Yeah. Actually there is a parameterized constructor added.

Ah I see, I didn't see it because constructors should be grouped together after the field declarations ;)

jdamerow · 2022-05-25T20:28:35Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/ExhibitionLanguage.java

+
+    @Id
+    @GeneratedValue(generator = "exhibit_language_id_generator")
+    @GenericGenerator(name = "exhibit_language_id_generator", parameters = @Parameter(name = "prefix", value = "EXH"), strategy = "edu.asu.diging.vspace.core.data.IdGenerator")


EXH is the prefix for exhibitiion objects. here it should be a prefix for language objects.

jdamerow · 2022-05-25T20:29:09Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+    @Autowired
+    private ExhibitionLanguageConfig exhibitionLanguageConfig;
+
+    private final Logger logger = LoggerFactory.getLogger(getClass());


llogger decllarations should be first in the class

jdamerow · 2022-05-25T20:30:07Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+                        .stream().filter(map-> code.equalsIgnoreCase((String) map.get("code")))
+                        .map(language -> { 
+                            ExhibitionLanguage exhibitionLanguage =   new ExhibitionLanguage((String) language.get("label"),
+                                    (String) language.get("code"), exhibition);


same as before, hard-coded strings should be in constants

jdamerow · 2022-05-25T20:47:53Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+            if(defaultLanguage!=null) {
+                codes.add(defaultLanguage);
+            }
+            codes.forEach(code -> {


I think I would turn this around and iterate over exhibitionLanguageConfig.getExhibitionLanguageList(). You would only need one stream and filter for everything in the codes list. So only one iteration instead of multiple if I'm not mistaken.

jdamerow · 2022-05-27T15:59:34Z

vspace/src/main/java/edu/asu/diging/vspace/config/ExhibitionLanguageConfig.java

+     */
+    @PostConstruct
+    public void init() {
+        for(Iterator it = ((AbstractEnvironment) environment).getPropertySources().iterator(); it.hasNext(); ) {


Can't you use get here: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/core/env/MutablePropertySources.html#get-java.lang.String-

jdamerow · 2022-05-27T16:43:07Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/ExhibitionLanguage.java

+
+    public void setDefault(boolean isDefault) {
+        this.isDefault = isDefault;
+    }


getter and setters should be in order of variable declaration. hashcode/equals should be after getters/setters.

jdamerow · 2022-05-27T16:46:08Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+            if(CollectionUtils.isNotEmpty(exhibitionLanguageConfig.getExhibitionLanguageList())) {
+                List<ExhibitionLanguage> exhibitionLanguages =   exhibitionLanguageConfig.getExhibitionLanguageList()
+                        .stream().filter(languageMap -> codes.contains(languageMap.get(ConfigConstants.CODE)))
+                        .map(map -> {


map (as variable name) is a language, isn't it? It should have a. better name.

jdamerow · 2022-05-27T16:46:55Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+    @Override
+    public void updateExhibitionLanguages(Exhibition exhibition, List<String> codes, String defaultLanguage) {
+        if(CollectionUtils.isNotEmpty(codes)) {
+            if(defaultLanguage!=null) {


&& !codes.contains(defaultLanguage) or you might end up with the same code twice?

jdamerow · 2022-05-27T16:47:47Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+                        }).collect(Collectors.toList());
+
+                if(CollectionUtils.isNotEmpty(exhibitionLanguages)) {
+                    logger.info("Updating Exhibition with Languages" + codes);


remove; it doesn't add any value. If at all, this should be trace.

jdamerow · 2022-05-27T16:48:38Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+                    exhibition.getLanguages().addAll(exhibitionLanguages);
+                }
+            } else {
+                logger.error("Language list configuration not available");


in this case we probably want to throw an exception since that would indicate a faulty installation.

jdamerow · 2022-05-27T16:51:15Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/ExhibitionConfigurationController.java


    @RequestMapping("/staff/exhibit/config")
    public String showExhibitions(Model model) {
        // for now we assume there is just one exhibition
        IExhibition exhibition = exhibitManager.getStartExhibition();
        if(exhibition!=null) {
            model.addAttribute("exhibition", exhibition);
+            if(exhibition.getLanguages() != null ) {
+                model.addAttribute("mappedLanguages", exhibition.getLanguages()
+                        .stream().map(language -> language.getLabel()).collect(Collectors.toList()));


what are "mappedLanguages"? and why not putting the language list in the model (not just the labels), you probably need them somewhere (or if not, it's just a .label more to show the language names).

mappedLanguages is just used to highlight the already mapped languages to an exhibition, in the selection dropdown. This is to avoid adding any more logic in front end.
The entire language object is being sent to populate the dropdown in "languageList".

ah, makes sense. It needs a better name though. It's not clear what "mappedLanguages" would be in this context.

jdamerow · 2022-05-27T16:52:51Z

vspace/src/test/java/edu/asu/diging/vspace/core/services/impl/ExhibitionLanguageTest.java

+import edu.asu.diging.vspace.core.model.impl.Exhibition;
+import edu.asu.diging.vspace.core.model.impl.ExhibitionLanguage;
+
+public class ExhibitionLanguageTest {


Tests class should be named ClassToBeTestedNameTest

…tion

jdamerow · 2022-06-08T18:27:23Z

...ce/src/main/java/edu/asu/diging/vspace/core/exception/LanguageListConfigurationNotFound.java

+
+    /**
+     * 
+     */


unless you intend to add javadoc here, remove

jdamerow · 2022-06-08T18:39:46Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/ExhibitionLanguage.java

+import org.hibernate.annotations.Parameter;
+
+@Entity
+public class ExhibitionLanguage {


interface is missing

jdamerow · 2022-06-08T18:41:57Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+     */
+    @Override
+    public void updateExhibitionLanguages(Exhibition exhibition, List<String> codes, String defaultLanguage) throws LanguageListConfigurationNotFound {
+        if(CollectionUtils.isNotEmpty(codes)) {


if you turn the if condition around (if codes is empty) you can avoid such a big if block by returning right away.

jdamerow · 2022-06-08T18:42:37Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+            }
+
+
+            if(CollectionUtils.isNotEmpty(exhibitionLanguageConfig.getExhibitionLanguageList())) {


check this right at the beginning. no need to do anything if there is no exhibition language list.

jdamerow · 2022-06-08T18:47:00Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+                (String) languageMap.get(ConfigConstants.CODE), exhibition);
+
+        int index =  exhibition.getLanguages().indexOf(exhibitionLanguage);
+        if( index < 0 ) {


why not use exhibition.getLanguages().contains()?

I would need the object to update isDefault to false in case a different default language is selected by user. Hence using index.

jdamerow · 2022-06-08T18:47:18Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+        if( index < 0 ) {
+            exhibition.getLanguages().add(exhibitionLanguage);
+        } 
+        else {


please follow our style guide

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

jdamerow · 2022-06-08T18:52:39Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/ExhibitionConfigurationController.java


    @RequestMapping("/staff/exhibit/config")
    public String showExhibitions(Model model) {
        // for now we assume there is just one exhibition
        IExhibition exhibition = exhibitManager.getStartExhibition();
        if(exhibition!=null) {
            model.addAttribute("exhibition", exhibition);
+            if(exhibition.getLanguages() != null ) {
+                model.addAttribute("mappedLanguages", exhibition.getLanguages()
+                        .stream().map(language -> language.getLabel()).collect(Collectors.toList()));


ah, makes sense. It needs a better name though. It's not clear what "mappedLanguages" would be in this context.

jdamerow · 2022-06-13T20:10:47Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/Exhibition.java

+        if (getClass() != obj.getClass())
+            return false;
+        Exhibition other = (Exhibition) obj;
+        return Objects.equals(id, other.id);


merge with line 154

jdamerow · 2022-06-13T20:20:47Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+    @Override
+    public void updateExhibitionLanguages(Exhibition exhibition, List<String> codes, String defaultLanguage) throws LanguageListConfigurationNotFound {
+        if(CollectionUtils.isEmpty(exhibitionLanguageConfig.getExhibitionLanguageList())) {
+            throw new LanguageListConfigurationNotFound("Exhibition Language Configuration not found");


I think I would make LanguageListConfigurationNotFound a RuntimeException. If this happens it means that the installation is incomplete or something else is going wrong and there is really no good way out. So I'd make it a runtime exception and not declare or handle the exception. Then just add a handler to the ExhibitionExceptionHandler that handles all runtime exceptions by showing a 500 page.

I have added it to the exception handler. The exception is thrown and not being handled. Is there any thing else to be done for this?

…ndler

jdamerow · 2022-06-17T17:49:56Z

vspace/src/main/java/edu/asu/diging/vspace/web/exception/ExhibitionExceptionHandler.java

+        modelAndView.addObject("showAlert", true);
+        modelAndView.addObject("message", ex.getMessage());
+        logger.info("LanguageListConfigurationNotFound Occured:: URL=" + request.getRequestURL());
+        logger.info("Code:: "+language_list_configuration_not_found+" Message:: " + ex.getMessage());


you want to make sure to log the stacktrace

jdamerow · 2022-06-17T17:50:55Z

vspace/src/main/java/edu/asu/diging/vspace/core/services/impl/ExhibitionManager.java

+     * @throws LanguageListConfigurationNotFound 
+     */
+    @Override
+    public void updateExhibitionLanguages(Exhibition exhibition, List<String> codes, String defaultLanguage) throws LanguageListConfigurationNotFound {


the exception should be a runtime exception and wouldn't need to be declared.

jdamerow · 2022-06-17T17:52:55Z

...ce/src/main/java/edu/asu/diging/vspace/core/exception/LanguageListConfigurationNotFound.java

@@ -0,0 +1,27 @@
+package edu.asu.diging.vspace.core.exception;
+
+public class LanguageListConfigurationNotFound  extends Exception {


this should be a runtime exception and exception class names should end in Exception

jdamerow · 2022-06-17T17:53:46Z

Make it so, Jenkins.

jdamerow · 2022-06-17T17:57:47Z

Besides that, looks good.

jdamerow · 2022-06-20T15:17:02Z

vspace/src/main/java/edu/asu/diging/vspace/core/model/impl/Exhibition.java

+        if (getClass() != obj.getClass())
+            return false;
+        return Objects.equals(id, ((Exhibition) obj).id);
+    }


sorry, I didn't see this before but first all getters/setters then hashcode/equals methods.

jdamerow · 2022-06-20T15:23:03Z

vspace/src/main/java/edu/asu/diging/vspace/web/exception/ExhibitionExceptionHandler.java

+        modelAndView.addObject("showAlert", true);
+        modelAndView.addObject("message", ex.getMessage());
+        logger.info("LanguageListConfigurationNotFound Occured:: URL=" + request.getRequestURL());
+        logger.info("Code:: "+language_list_configuration_not_found+" Cause:: " + ex);


that should be logged as error

jdamerow · 2022-06-20T15:24:38Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/ExhibitionConfigurationController.java

-            RedirectAttributes attributes) throws IOException {
+            @RequestParam("exhibitLanguage") List<String> languages,
+            @RequestParam("defaultExhibitLanguage") String defaultLanguage,
+            RedirectAttributes attributes) throws IOException, LanguageListConfigurationNotFoundException {


runtime exceptions don't need to be declared

jdamerow · 2022-06-21T15:07:47Z

vspace/src/main/java/edu/asu/diging/vspace/web/staff/ExhibitionConfigurationController.java

@@ -59,13 +82,16 @@ public String showExhibitions(Model model) {
     * @param spaceParam
     * @param attributes
     * @return
+     * @throws LanguageListConfigurationNotFoundException 


method is not throwing this anymore

jdamerow · 2022-06-21T15:10:40Z

Make it so, Jenkins.

jdamerow · 2022-06-21T15:13:36Z

After that should be good.

jdamerow · 2022-06-24T15:37:32Z

Make it so, Jenkins.

jdamerow · 2022-06-24T15:42:19Z

There seems to be a bug. I have 4 languages currently selected, I've deselected 2 and saved. But the four are still selected when the page reloads.

jdamerow · 2022-06-28T16:34:20Z

Make it so, Jenkins.

Prachi Kharge added 6 commits May 12, 2022 16:25

[story/VSPC-213]added exhibitionlanguage codes

5b25410

[story/VSPC-213] added configuration for language codes and labels

c3f1f8b

[story/VSPC-213] added support for multiple exhibition languages

c770d63

[story/VSPC-213] added test cases

959b957

[story/VSPC-213] added code for default language

bf91424

[VSPC-213] refactor

cbf9519

Prachi Kharge added 3 commits May 23, 2022 16:20

[story/VSPC-213] codefactor

f3285d4

[story/VSPC-213] refactor

7ac5aed

[story/VSPC-213] revert unwanted file changes

ff7a491

jdamerow requested changes May 25, 2022

View reviewed changes

jdamerow closed this May 25, 2022

Prachi Kharge added 2 commits May 26, 2022 12:11

[story/VSPC-213] review comments

1e883bd

[story/VSPC-213] modified prefix of exhibition language

e313921

pkharge reopened this May 26, 2022

jdamerow requested changes May 27, 2022

View reviewed changes

jdamerow closed this May 27, 2022

Prachi Kharge added 2 commits June 6, 2022 16:25

[story/VSPC-213] added exceptions. modified exhibition language config

52e2fd5

[story/VSPC-213] added check for only one default language per exhibi…

4575ca4

…tion

pkharge reopened this Jun 7, 2022

Prachi Kharge added 4 commits June 7, 2022 13:51

[story/VSPC-213] code factor

f57fa0a

[story/VSPC-213] code factor

7a8e502

[story/VSPC-213] refactor

0784a7b

[story/VSPC-213] refactor

5382226

jdamerow requested changes Jun 8, 2022

View reviewed changes

jdamerow closed this Jun 8, 2022

[story/VSPC-213] added interface for Exhibition language

00cc39f

pkharge reopened this Jun 10, 2022

[story/VSPC-213] code factor

98a3eb8

jdamerow requested changes Jun 13, 2022

View reviewed changes

Prachi Kharge added 2 commits June 16, 2022 16:23

[story/VSPC-213] langauge list config not found added to exception ha…

00d8754

…ndler

[story/VSPC-213] refactor

1c566b7

pkharge reopened this Jun 16, 2022

jdamerow requested changes Jun 17, 2022

View reviewed changes

jdamerow closed this Jun 17, 2022

[story/VSPC-213] modified language list config not found exception

39b9f8b

pkharge reopened this Jun 17, 2022

[story/VSPC-213] refactor

bcd81b6

jdamerow requested changes Jun 20, 2022

View reviewed changes

jdamerow closed this Jun 20, 2022

[story/VSPC-213] review comments

72f033b

pkharge reopened this Jun 20, 2022

jdamerow requested changes Jun 21, 2022

View reviewed changes

jdamerow closed this Jun 21, 2022

[story/VSPC-213] removed LanguageListConfigException declaration

dcac0ab

pkharge reopened this Jun 21, 2022

[story/VSPC-213] refactor

33712c2

jdamerow closed this Jun 24, 2022

[story/VSPC-213] remove exhibitionlanguage if unselected

550a4ee

pkharge reopened this Jun 27, 2022

[story/VSPC-213] added test case

4cea3c2

jdamerow merged commit 1181039 into develop Jun 28, 2022

jdamerow deleted the story/VSPC-213 branch June 28, 2022 17:39

		}


		if(CollectionUtils.isNotEmpty(exhibitionLanguageConfig.getExhibitionLanguageList())) {

		@@ -0,0 +1,27 @@
		package edu.asu.diging.vspace.core.exception;

		public class LanguageListConfigurationNotFound extends Exception {

Story/vspc 213 #302

Story/vspc 213 #302

Conversation

pkharge commented May 23, 2022

Guidelines for Pull Requests

Please provide a brief description of your ticket

Anything else the reviewer needs to know?

diging-jenkins commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdamerow commented Jun 17, 2022

jdamerow commented Jun 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdamerow commented Jun 21, 2022

jdamerow commented Jun 21, 2022

jdamerow commented Jun 24, 2022

jdamerow commented Jun 24, 2022

jdamerow commented Jun 28, 2022