Story/vspc 147 #228

pradeep-017 · 2021-07-06T17:33:06Z

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)

Completed changes for changes for Bibliographies and References.

Anything else the reviewer needs to know?

... describe here ...

…k in slides

… page - Work in progress

… on slide page + Code cleanup

…aces-2.0 into story/VSPC-147

… them, fixed code indentations and div alignments

…code cleanup

… shown on public page

…ography not detected by zotero extension

…t it

…detection by zotero - work in progress

…ity completed

…aphy, handling null cases of path variables, code cleanup

…rategy Design pattern

… changes

…ged Biblio controller and forms - Saving is working - Work in Progress

…io block

jdamerow

the issue with toString and the html method is still in there. any kind of formatting should happen in the template or through external classes, not the model class.

jdamerow · 2025-02-07T17:48:13Z

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

+                + type + ", " + note;
+    }
+
+    public String htmlRenderedReference() {


what is this for?

it parses the text returned by toString() (which in this case is "bibliography title: " + biblioTitle + " description: " + description) as Markdown, and then render that Markdown as HTML. it is used in modules for biblio block and reference block to render "Biblio block" and references, its just returning the string

ya, that should not be done in the domain model class. that needs to be done in the template or through an extra class.

jdamerow · 2025-02-07T17:48:28Z

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

+        Parser parser = Parser.builder().build();
+        Node document = parser.parse(toString());
+        HtmlRenderer renderer = HtmlRenderer.builder().build();
+        return renderer.render(document);


what is this for?

Same as before, is this how it should be done? Or should I change it? If yes, then let me know how

addressed comment

jdamerow · 2025-02-21T14:57:55Z

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

+    @Autowired
+    private IContentBlockManager contentBlockManager;
+
+    @RequestMapping(value = "/staff/module/{id}/slide/{slideId}/bibliography/{biblioId}/references", method = RequestMethod.POST)


should be .../references/add

jdamerow · 2025-02-21T14:58:11Z

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

+        String type = rootNode.get("type").asText();
+        String note = rootNode.get("note").asText();
+
+        IBiblioBlock biblio = contentBlockManager.getBiblioBlock(biblioId);


this should probably be part of the createReference method

jdamerow · 2025-02-21T15:00:56Z

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

+    @Autowired
+    private IReferenceManager referenceManager;
+
+    @RequestMapping(value = "/staff/module/{moduleId}/slide/{id}/bibliography/{biblioId}/reference/edit", method = RequestMethod.POST)


should be consistent, either
.../reference/edit and .../reference/add
or
.../references/edit and .../references/add

jdamerow · 2025-02-21T15:01:51Z

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

+
+        IReference reference = referenceManager.getReference(id);
+        if (reference == null) {
+            return new ResponseEntity<String>(HttpStatus.NOT_FOUND);


needs to log why this is returned

Addressed, but I don't see the newly added line on the review side.

codefactor.io issue

jdamerow · 2025-02-21T15:02:10Z

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

+        reference.setEditors(editor);
+        reference.setType(type);
+        reference.setNote(note);
+        referenceManager.updateReference((Reference) reference);


it should not be necessary to cast to Reference here

jdamerow · 2025-02-21T15:03:26Z

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

+            	    $(".EasyMDEContainer").remove();
+	         	    $(e.target).closest('.referenceDivStaff').addClass("open");
+	         	    var description = $("div.open p:eq(0)").text();
+             	    defaultValue = description;


what's up with the indentation in the couple of lines above?

jdamerow · 2025-02-21T15:03:41Z

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

+		    	    var alert = $('<input type="hidden" id="currentBiblioId">');
+		    		/* $(alert).attr( 'value', $(this).parent().parent().parent().attr("id"));*/
+		    		$(alert).attr( 'value', $(e.target).closest('.biblioDivStaff').attr("id"));
+		    		$('.modal-footer').append(alert); 


indentation (all the above)

jdamerow · 2025-02-21T15:03:50Z

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

+             	    //showing the edit bibliography form with all the respective values
+		    	    $("#editReferenceAlert").show();
+		    	    var alert = $('<input type="hidden" id="currentBiblioId">');
+		    		/* $(alert).attr( 'value', $(this).parent().parent().parent().attr("id"));*/


remove if not needed

jdamerow · 2025-02-21T15:07:44Z

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

+         						+ 'Issue: <span>' + data.issue + '</span>, Pages: <span>' + data.pages + '</span>, '
+         						+ 'Editors: <span>' + data.editors + '</span>, Type: <span>' + data.type + '</span>, '
+         						+ 'Note: <span>' + data.note + '</span>'
+                         		+ '<input type="hidden" id="deleteReferenceId" value="'+ data.id +'" /><a class="btn deleteReference" href="javascript:;" style="position: absolute; right:0px;"><i style="color: var(--error);" class="fas fa-trash-alt"></i></a></p></div>');


doesn't this mean that every time I add a new reference, the new div will have an input with the same id? IDs have to be unique.

the ids are different

how is that possible if it's hard-coded: id="deleteReferenceId"

ids are different for difference references

for the input as well, the values of the unique id is being stored, and is called while deleting it

Isn't this code creating an input field for every reference with the id "deleteReferenceId" (and a unique value)? This is what the code looks like, and if that is the case, then the id is not unique (it's "deleteReferenceId" for every hidden input field).

jdamerow · 2025-02-21T15:11:09Z

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

+		    	    $("#editReferenceAlert").show();
+		    	    var alert = $('<input type="hidden" id="currentBiblioId">');
+		    		/* $(alert).attr( 'value', $(this).parent().parent().parent().attr("id"));*/
+		    		$(alert).attr( 'value', $(e.target).closest('.biblioDivStaff').attr("id"));


this seems unnecessarily complicated and brittle.
a) why is the variable called "alert"? It does not contain an alert, does it?
b) if I understand this right, all that this code is trying to do is that the id of the reference that should be deleted in the delete form, right? If so, the block that was clicked on should just have a data attribute that holds the id of the reference that should be deleted. Then get that value here and set it in the input.
c) The input element should probably be already in the modal, and then just set the value here. No need to create a new input element every time.

The same is true for all the other cases where there is similar code here in the file.

jdamerow · 2025-03-12T19:12:26Z

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

+        reference.setEditors(editor);
+        reference.setType(type);
+        reference.setNote(note);
+        reference.getBiblios().add((BiblioBlock) biblio);


It should not be necessary to cast to BiblioBlock here

jdamerow · 2025-03-12T19:16:17Z

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

+    @Override
+    public IReference createReference(String biblioId, String title, String author,String year,String journal, String url, String volume,String issue, String pages,String editor, String type, String note) {
+    	IReference reference = new Reference();
+        IBiblioBlock biblio = contentBlockManager.getBiblioBlock(biblioId);


this line should be right before line 46 or even merged

jdamerow · 2025-03-12T19:19:11Z

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

+
+    @RequestMapping(value = "/staff/module/{moduleId}/slide/{id}/bibliography", method = RequestMethod.POST)
+    public ResponseEntity<String> addBiblioBlock(@PathVariable("id") String slideId,
+            @PathVariable("moduleId") String moduleId, @RequestBody String biblioBlockData) throws JsonProcessingException {


Can't you have spring parse the json into an object (just create a BiblioBlockData class with the necessary fields and use it here)? This way you can drop line 35-41.

jdamerow · 2025-03-12T19:19:17Z

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

+        String description = rootNode.get("description").asText();
+        Integer contentOrder = contentBlockManager.findMaxContentOrder(slideId);
+        contentOrder = contentOrder == null ? 0 : contentOrder + 1;
+        System.out.println("/n/n/n/n"+contentOrder + "/n/n/n/n/n");


jdamerow · 2025-03-12T19:19:45Z

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

+    @RequestMapping(value = "/staff/module/{id}/slide/{slideId}/bibliography/{biblioId}/reference/add", method = RequestMethod.POST)
+    public ResponseEntity<String> addReference(@PathVariable("id") String moduleId, @PathVariable("slideId") String slideId, 
+            @PathVariable("biblioId") String biblioId, 
+            @RequestBody String reference, Model model) throws JsonProcessingException {


same here, let spring do the parsing.

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

jdamerow · 2025-03-12T19:20:37Z

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

+
+        String biblioTitle = rootNode.get("biblioTitle").asText();
+        String description = rootNode.get("description").asText();
+        String id = rootNode.get("id").asText();


same as above

jdamerow · 2025-03-12T19:22:11Z

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

+
+        IReference reference = referenceManager.getReference(id);
+        if (reference == null) {
+            return new ResponseEntity<String>(HttpStatus.NOT_FOUND);


codefactor.io issue

jdamerow · 2025-03-12T19:25:38Z

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

+         						+ 'Issue: <span>' + data.issue + '</span>, Pages: <span>' + data.pages + '</span>, '
+         						+ 'Editors: <span>' + data.editors + '</span>, Type: <span>' + data.type + '</span>, '
+         						+ 'Note: <span>' + data.note + '</span>'
+                         		+ '<input type="hidden" id="deleteReferenceId" value="'+ data.id +'" /><a class="btn deleteReference" href="javascript:;" style="position: absolute; right:0px;"><i style="color: var(--error);" class="fas fa-trash-alt"></i></a></p></div>');


Isn't this code creating an input field for every reference with the id "deleteReferenceId" (and a unique value)? This is what the code looks like, and if that is the case, then the id is not unique (it's "deleteReferenceId" for every hidden input field).

jdamerow

still codefactor issues

jdamerow · 2025-03-25T19:03:33Z

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

+            referenceData.getNote()
+        );
+
+        return ResponseEntity.ok((Reference) ref);


Do you have to cast here? I would assume you can pass in IRefrence.

its not working if I am passing the IReference. Type mismatch: cannot convert from ResponseEntity to ResponseEntity
but why are we avoiding TypeCasting?

jdamerow · 2025-03-25T20:55:35Z

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

+         		$("#confirmDeleteChoiceBlockAlert").find('input').remove();
+         		$("#confirmDeleteRefAlert").hide();
+         		var refId = $('.deleteReferenceId').attr('value');
+         		$('.deleteReferenceId').remove();


I haven't tested this yet, but what I expect:

I create multiple references, which means multiple <input type="hidden" class="deleteReferenceId" ...> will be created.

the above lines will probably take the first input they find with class deleteReferenceId

from that input, the id is used to delete a reference.
That can't be right?

deletetion is working properly, I checked with different combinations

ajay3568yadav · 2025-03-26T23:36:31Z

I don't see any codefactor issues?

pradeep-017 and others added 30 commits May 5, 2021 16:28

[VSPC-147] Intial changes and new files created for Bibliography bloc…

36843b0

…k in slides

Merge branch 'develop' into story/VSPC-147

5df90f2

[VSOC-147] Edit Bibliography controller created and using it in slide…

afbde02

… page - Work in progress

[VSPC-147] code cleaning for slide.html

4e7397f

Merge branch 'develop' into story/VSPC-147

73bda55

[VSPC-147] Delete functionality for bibliography and button alignment…

30f114b

… on slide page + Code cleanup

[VSPC-147] Code indentation changes for slide.html

4eb9091

[VSPC-147] Code indentation fix for slide.html

9ccba7f

[VSPC-147] Adding more fields for bibliography - Work in progress

c6a3aac

Merge branch 'story/VSPC-147' of https://github.com/diging/virtual-sp…

806c6f3

…aces-2.0 into story/VSPC-147

[VSPC-147] Added new fields and worked on CRUD opeerations to work on…

ecc0a56

… them, fixed code indentations and div alignments

[VSPC-147] Tests for bibliography feature on slides-Work in progress

d79ca74

[VSPC-147] Test cases for Bibliography

e78221a

Merge branch 'develop' into story/VSPC-147

0efa1c4

[VSPC-147] csrf token passing via headers enabled for ajax calls and …

e35d814

…code cleanup

[VSPC-147] Image icon changed for bibliography and bibliographies now…

ef356a5

… shown on public page

[VSPC-147] Corrected id tags for slide contents on public page, Bibli…

1472da8

…ography not detected by zotero extension

[VSPC-147] Changes for bibliography citations and making Zotero detec…

6cfd2dc

…t it

[VSPC-147] Changes for bibliography metadata view on public page for …

1f705ce

…detection by zotero - work in progress

[VSPC-147] Detection of Bibliographies by Zotero - General functional…

cd5de7c

…ity completed

Merged develop into story/VSPC-147

23c7a6e

[VSPC-147] Code changes for returning json afetr creation of Bibliogr…

050434c

…aphy, handling null cases of path variables, code cleanup

[VSPC-147] Json response checks and handling for bibliography creation

6a97e07

[VSPC-147] Updated biblio exception class for code factor error check

b911508

[VSPC-147] Exposing Biblipgraphy metadata to be picked up by zotero

fa4b232

[VSPC-147] Exposing bibliography metadata to be picked by zotero - St…

4ab456d

…rategy Design pattern

[VSPC-147] Intriducing Reference class under Bibliography and related…

97de8e4

… changes

[VSPC-147] Added Reference factory, manager, controller classes. Chan…

4701276

…ged Biblio controller and forms - Saving is working - Work in Progress

[VSPC-147] changes for CRUD on bibliography block and references

b6a6931

[VSPC-147] AddReference controller and bringing references under bibl…

e011fd2

…io block

jdamerow closed this Jan 29, 2025

VSPC-147 review comments

fd12d7a

ajay3568yadav reopened this Jan 31, 2025

jdamerow requested changes Feb 7, 2025

View reviewed changes

jdamerow closed this Feb 7, 2025

VSPC_147 review comments on html rendering

81003bc

ajay3568yadav reopened this Feb 7, 2025

jdamerow requested changes Feb 21, 2025

View reviewed changes

jdamerow closed this Feb 21, 2025

VSPC-147 review comments

c07b3fa

ajay3568yadav reopened this Feb 26, 2025

ajay3568yadav closed this Feb 26, 2025

ajay Yadav and others added 3 commits February 26, 2025 15:06

VSPC-147 review comments and content order fix

f783d66

VSPC-147 content order issue addressing

0465dcd

VSPC-147 fixed contentOrder

e927036

ajay3568yadav reopened this Mar 5, 2025

ajay3568yadav closed this Mar 7, 2025

VSPC-147 review comments

ccc2a21

ajay3568yadav reopened this Mar 7, 2025

jdamerow requested changes Mar 12, 2025

View reviewed changes

jdamerow closed this Mar 12, 2025

ajay3568yadav added 2 commits March 12, 2025 16:39

VSPC-147 review comments on parsing using Spring

8aff7e5

VSPC-147 review comments addressed

3252d47

ajay3568yadav reopened this Mar 13, 2025

jdamerow requested changes Mar 25, 2025

View reviewed changes

jdamerow closed this Mar 25, 2025

ajay3568yadav reopened this Mar 26, 2025

ajay3568yadav closed this Mar 26, 2025

ajay3568yadav reopened this Mar 26, 2025

Story/vspc 147 #228

Are you sure you want to change the base?

Story/vspc 147 #228

Conversation

pradeep-017 commented Jul 6, 2021 • edited by jdamerow Loading

Guidelines for Pull Requests

Please provide a brief description of your ticket

Anything else the reviewer needs to know?

jdamerow left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajay3568yadav Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajay3568yadav commented Mar 26, 2025

pradeep-017 commented Jul 6, 2021 •

edited by jdamerow

Loading

ajay3568yadav Mar 26, 2025 •

edited

Loading