Story/vspc 171 #266

glend17 · 2021-11-12T23:45:55Z

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)

https://diging.atlassian.net/browse/VSPC-171

Anything else the reviewer needs to know?

I have added a new content block type called Space Block, which has the same properties as the content block with an additional parameter for linking to a space. I have allowed the selection of a space for the Staff User from a drop down

…ace content block

diging-jenkins · 2021-11-12T23:45:59Z

Can one of the admins verify this patch?

jdamerow · 2022-01-12T21:31:27Z

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

+            }
+
+        }
+        return selectedSpaceForSpaceBlock;


what is this method for? why does it retrieve space blocks again? (also if you. need to check the type of an object you should use instanceof).

Removed this method as it was not needed

jdamerow · 2022-01-12T21:32:36Z

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

+
+        } catch (BlockDoesNotExistException e) {
+            logger.warn("Text Id does not exist, bad request.", e);
+            return new ResponseEntity<String>(HttpStatus.BAD_REQUEST);


if a block doesn't exist, there should be a 404 returned

jdamerow · 2022-01-12T21:33:31Z

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

+        List<ISpace> spaces = new ArrayList<ISpace>();
+        spaceRepo.findAll().forEach(s -> {
+            spaces.add(s);
+        });


why are the spaces added to a new list? doesn't findAll return a list?

jdamerow · 2022-01-12T21:34:55Z

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

        model.addAttribute("contentCount",slideContents.size()>0 ? slideContents.get(slideContents.size()-1).getContentOrder() : 0);
        if(slideManager.getSlide(id) instanceof BranchingPoint) {
            model.addAttribute("choices", ((IBranchingPoint)slide).getChoices());
-        }           
+        }      
+        System.out.println("the map is"+ selectedSpaceForSpaceBlock.get("CON000000007"));


Really? you even checked the box saying "I do not have any sysout statements in my code or commented out code that isn’t needed anymore" ;)

Sorry about that, have removed it now

jdamerow · 2022-01-12T21:35:56Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+            		var blockId = $(this).closest("div").attr("id");
+            		console.log('block id is'+ blockId)
+            		var selectedSpace = selectedSpaceForSpaceBlock[blockId]
+            		console.log('selected space is '+selectedSpace)


all console.log

jdamerow · 2022-01-12T21:36:28Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+                    $(".open").css('display', 'none');
+                    $(".open").css('overflow-y', 'scroll');
+                    $(".open").css('height', '400px');
+                }


fix indentations in above block

The indentation appears correctly in eclipse i am not sure why it appears like this here.

jdamerow · 2022-01-12T21:37:57Z

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

        model.addAttribute("slideContents", slideContents);
+        model.addAttribute("selectedSpaceForSpaceBlock", selectedSpaceForSpaceBlock);


what are these needed for? each space block should have a space property with the linked space.

Removed this, initially had included this to make the user know which is the current selected space for a space content block, but I have figured a way around this.

jdamerow · 2022-02-23T20:14:46Z

Resolve conflicts please

jdamerow · 2024-09-27T17:39:30Z

Make it so, Jenkins.

jdamerow · 2024-09-27T17:46:06Z

current space should be selected in dropdown

jdamerow · 2024-10-25T21:03:43Z

Make it so, Jenkins.

jdamerow · 2024-10-25T21:08:33Z

Make it so, Jenkins.

jdamerow · 2024-10-25T21:14:52Z

Some slides don't seem to work: https://diging-dev.asu.edu/vspace-dev/staff/module/MOD000000002/slide/SLI000000021

Caused by: org.springframework.expression.spel.SpelEvaluationException: EL1007E: Property or field 'name' cannot be found on null
	at org.springframework.expression.spel.ast.PropertyOrFieldReference.readProperty(PropertyOrFieldReference.java:213)
	at org.springframework.expression.spel.ast.PropertyOrFieldReference.getValueInternal(PropertyOrFieldReference.java:104)
	at org.springframework.expression.spel.ast.PropertyOrFieldReference.access$000(PropertyOrFieldReference.java:51)
	at org.springframework.expression.spel.ast.PropertyOrFieldReference$AccessorLValue.getValue(PropertyOrFieldReference.java:406)
	at org.springframework.expression.spel.ast.CompoundExpression.getValueInternal(CompoundExpression.java:92)
	at org.springframework.expression.spel.ast.SpelNodeImpl.getValue(SpelNodeImpl.java:112)
	at org.springframework.expression.spel.standard.SpelExpression.getValue(SpelExpression.java:337)
	at org.thymeleaf.spring5.expression.SPELVariableExpressionEvaluator.evaluate(SPELVariableExpressionEvaluator.java:265)
	... 130 more

jdamerow · 2024-10-30T19:23:04Z

Make it so, Jenkins.

jdamerow · 2024-10-30T19:28:12Z

Make it so, Jenkins.

jdamerow · 2024-10-30T19:29:27Z

Make it so, Jenkins.

jdamerow · 2024-10-30T19:31:49Z

Make it so, Jenkins.

jdamerow · 2024-10-30T19:33:16Z

Make it so, Jenkins.

jdamerow · 2024-10-30T19:37:38Z

Make it so, Jenkins.

jdamerow · 2024-11-01T17:01:18Z

Make it so, Jenkins.

jdamerow · 2024-11-01T18:13:52Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+                    url: "[(@{'/staff/module/'+${module.id}+'/slide/'+${slide.id}+'/space/'})]" + blockId,
+                    type: 'GET',
+                    success: function(result) {
+                    	if (result != null && result.space != null && result.space.name != null){


I don't think result.space.name != null is needed here? If the space name is null, it shouldn't prevent the space block from being created. However, if there is no title, there should be some dummy text (in the space selection list as well as when displaying the block/link), e.g. ""

jdamerow · 2024-11-01T18:14:21Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+                            currentSpaceName = result.space.name;
+                            $("#selectedSpace").text(currentSpaceName);
+                            $("#editSpaceBlock").text(result.title);
+                            currentSpaceId = result.space.id;


put this line together with line 162

and honestly, not sure if you even need extra variables here. You can just use result.space.name and result.space.id everywhere.

jdamerow · 2024-11-01T18:17:39Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

-                	}
-
-                }
+            async function spaceBlockDoubleClick(e) {


why is this async?

jdamerow · 2024-11-01T18:20:29Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+                                $("#editSpaceName").append(newOption).trigger('change');
+                            } else {
+                                $("#editSpaceName").val(currentSpaceId).trigger('change');
+                            }


i'm so confused, what is this if block for?

This if block checks whether a specific option (with a value equal to currentSpaceId) already exists in the #editSpaceName dropdown menu. If the option is missing, a new one is created with currentSpaceName as the display text and currentSpaceId as the value.

jdamerow · 2024-11-01T18:21:42Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+					$("#confirmDeleteTextAlert").find('input').remove();
+					$("#confirmDeleteImageAlert").find('input').remove();
+					$("#confirmDeleteChoiceBlockAlert").find('input').remove();
+					$("#confirmDeleteSpaceAlert").hide();


indentation

jdamerow · 2024-11-01T18:24:07Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

@@ -991,6 +1181,107 @@
                    });
                });

+                function getSpaceById(spaceId, callback) {


get space by id should use an api call that actually gets the space by its id, not search all and then iterate over all!

jdamerow · 2024-11-01T18:24:51Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+                            let spaceName = null;
+                            for (let i = 0; i < result.length; i++) {
+                                const space = result[i];
+                                if (space.id === spaceId) {


where is spaceId defined?

jdamerow · 2024-11-01T18:25:29Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+                    });
+                }
+
+                function getCurrentSpace(blockId) {


why is this getting blockId but is called getCurrentSpace and what's the difference to getSpaceById?

jdamerow · 2024-11-01T18:27:14Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

+			            var spaceId = selectedSpace.options[selectedSpace.selectedIndex].value;
+			            formData.append('title', title);
+			            formData.append('spaceId', spaceId);
+			            getSpaceById(spaceId, function(spaceName){


for what does this need to be a callback?

In fact, why does getSpaceById to be called here? The space block should only need the space and a title, which you have after selecting a space, don't you?

jdamerow · 2024-11-01T18:29:26Z

vspace/src/main/webapp/WEB-INF/views/staff/modules/slides/slide.html

            var addVideoClk=0;
            var editVideoClk=0;
+            var currentBlockId;


I'm am fairly certain both currentSpaceId and currentBlockId do not need to be global variables.

without currentBlockId as a global variable close Space Block does not work as it is not able to get the id of the block, if you have any other suggestion then lmk.

glend17 added 11 commits October 27, 2021 12:17

[VSPC-171] Added new SpaceBlock

bc51fc4

[VSPC-171] Added SpaceBlock type functionality

7be602f

[VSPC-171] Adding SpaceBlock functionality

5efc1a9

[VSPC-171] Front-end changes

11aa526

[VSPC-171] Added drop down for staff to pick space to link to from sp…

1bb50c2

…ace content block

[VSPC-171] Bug checks

afb962c

[VSPC-171] Form fix

6ec14dd

[VSPC-171] Space content block changes on slide.html

4d5a7a7

[VSPC-171] Delete and edit related changes for space block

83b4e4d

[VSPC-171] Unit test

733d50f

[VSPC-171] dropdown will keep track of selected space block

dca101f

glend17 added 3 commits November 15, 2021 09:55

[VSPC-171] Code factor changes

5eb2f6e

[VSPC-171] PR code review change

1fda47d

[VSPC-171] Removing loggers

12aaaec

jdamerow requested changes Jan 12, 2022

View reviewed changes

jdamerow closed this Jan 12, 2022

glend17 added 8 commits February 17, 2022 11:05

[VSPC-171] PR Review comments

017bc82

[VSPC-171] Review comments

8801229

[VSPC-171] PR Review comments

82fc9c0

[VSPC-171] PR review comments

60d3fa1

[VSPC-171] Whitespace removal

b944cd7

[VSPC-171] Exhibition side link

fb785dc

[VSPC-171] PR Review comments addressed

4132c9d

[VSPC-171] PR review comments

95178b1

glend17 reopened this Feb 23, 2022

jdamerow closed this Feb 23, 2022

[VSPC-171] merging with develop

eb24c98

glend17 reopened this Feb 23, 2022

ajay3568yadav added 3 commits September 25, 2024 15:17

VSPC-171 review comments minor change

d630f15

VSPC-171 review comments minor change check

497b66c

VSPC-171 review comments minor change check

bcb2a82

jdamerow closed this Sep 27, 2024

ajay3568yadav added 2 commits September 27, 2024 12:49

VSPC-147 set selected space to the dropdown

eb6306d

[VSPC-171] fixed display of selected space

d334f31

ajay3568yadav reopened this Oct 2, 2024

jdamerow closed this Oct 25, 2024

VSPC-171 null result error fixed

1da97d9

ajay3568yadav reopened this Oct 30, 2024

ajay Yadav added 2 commits October 30, 2024 15:32

VSPC-171 testing for deployment

10f6d58

[VSPC-171] slide not showing up error:

9c110c2

jdamerow requested changes Nov 1, 2024

View reviewed changes

jdamerow closed this Nov 1, 2024

ajay Yadav added 3 commits November 1, 2024 12:43

VSPC-171 review comments

1a2ef85

VSPC-171 search space by id controller

f664e48

VSPC-171 review comments

1c4ac5f

ajay3568yadav reopened this Nov 8, 2024

		model.addAttribute("slideContents", slideContents);
		model.addAttribute("selectedSpaceForSpaceBlock", selectedSpaceForSpaceBlock);

Story/vspc 171 #266

Are you sure you want to change the base?

Story/vspc 171 #266

Conversation

glend17 commented Nov 12, 2021

Guidelines for Pull Requests

Please provide a brief description of your ticket

Anything else the reviewer needs to know?

diging-jenkins commented Nov 12, 2021

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 Feb 23, 2022

jdamerow commented Sep 27, 2024

jdamerow commented Sep 27, 2024

jdamerow commented Oct 25, 2024

jdamerow commented Oct 25, 2024

jdamerow commented Oct 25, 2024

jdamerow commented Oct 30, 2024

jdamerow commented Oct 30, 2024

jdamerow commented Oct 30, 2024

jdamerow commented Oct 30, 2024

jdamerow commented Oct 30, 2024

jdamerow commented Oct 30, 2024

jdamerow commented Nov 1, 2024

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