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

TRSA Branch for Feedback #6068

Open
wants to merge 75 commits into
base: develop
from

Conversation

@djbrooke
Copy link
Contributor

commented Aug 1, 2019

After discussion with @jonc1438 @akio-sone @scolapasta and @landreev, I'm going to put up this branch as a PR so that it can be tracked on the board and we can provide feedback through PR comments. We do not plan to merge this just yet.

akio-sone and others added some commits May 1, 2018

typo correction and enum handling
dataFile => dataTable
contin => CONTINUOUS, etc.
Merge pull request #1 from IQSS/develop
post-version 4.9 update

@djbrooke djbrooke added this to Code Review 🦁 in IQSS/dataverse via automation Aug 1, 2019

@coveralls

This comment has been minimized.

Copy link

commented Aug 1, 2019

Coverage Status

Coverage decreased (-0.1%) to 19.434% when pulling 86f3792 on OdumInstitute:trsa-api into 9f946f9 on IQSS:develop.

@pdurbin
Copy link
Member

left a comment

@akio-sone I left a lot of individual questions and comments in my review but here are some higher level ones:

  • What do we call this feature from the Dataverse perspective?
  • Is this branch far enough along that we could or should spin in up and attempt a demo like Jon has done? Can this be done using dataverse-ansible (or dataverse-kubernetes, or dataverse-docker)?
  • Can a "how to test" writeup be added?
  • We need to document new endpoints in API Guide (api/admin/trsaRegistration for example) and any changed endpoints. This could help with the "how to test" explanation.
  • What do you think of the diagram I made below? How accurate is it?

trsa

@@ -59,3 +60,4 @@ scripts/installer/default.config
# ignore UI testing related files
tests/node_modules
tests/package-lock.json
.factorypath

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

What is .factorypath and does it have anything to do with TRSA? Sorry to bike shed. 😄

@@ -28,6 +28,7 @@ RUN sudo -u postgres /usr/pgsql-9.6/bin/initdb -D /var/lib/pgsql/data

# copy configuration related files
RUN cp /tmp/dv/pg_hba.conf /var/lib/pgsql/data/
RUN cp /tmp/dv/postgresql.conf /var/lib/pgsql/data/

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

What is the purpose of this postgres config file?

@@ -75,7 +76,7 @@ ENV doi_username=${doi_username}
ENV doi_password=${doi_password}
COPY configure_doi.bash /opt/dv

# healthcheck for glassfish only (assumes modified domain.xml);
# healthcheck for glassfish only (assumes modified domain.xml);

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

All these whitespace changes are a little distracting when doing code review.

@@ -7,7 +7,7 @@ if [ ! -z "${DoiProvider}" ]; then
curl -X PUT -d ${DoiProvider} http://localhost:8080/api/admin/settings/:DoiProvider
fi
if [ ! -z "${doi_username}" ]; then
bin/asadmin create-jvm-options "-Ddoi.username=${doi_password}"
bin/asadmin create-jvm-options "-Ddoi.username=${doi_username}"

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

This looks like a bug fix, which is good, but it's unrelated to TRSA, right? Perhaps this should be in a separate, tiny pull request.

@@ -0,0 +1,185 @@
# (change requires restart)

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

I don't understand why this postgres config file is in here. What is it for? What does it do?



<h:outputStylesheet library="css" name="jsfcrud.css"/>
<h:outputScript library="js" name="jsfcrud.js"/>

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

Do we need a separate, tiny javascript file?

} else {
PF(dialog).hide();
}
}

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

This javascript file is pretty small. Should its only method be merged into some existing file?

This comment has been minimized.

Copy link
@mheppler

mheppler Aug 7, 2019

Contributor

These are questions best suited for #4181 or #2647 and not for #5213.

@@ -0,0 +1,82 @@
root {

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

I'll defer to @mheppler on this but I assume we might want to merge this CSS into structure.css.

This comment has been minimized.

Copy link
@mheppler

mheppler Aug 7, 2019

Contributor

We have plenty of other tiny CSS and JS files from various plugins. Nothing new and different to be concerned about. There are formatting questions with how the files are linked in the template, and their placement in the code, that said, these are all details that are waaaaay too granular for this initial prototype phase of code review.

@@ -385,6 +385,9 @@
<div class="file-icon-restricted-block" jsf:rendered="#{fileMetadata.restricted and fileDownloadHelper.canDownloadFile(fileMetadata) }">
<span class="icon-unlock text-success"/>
</div>
<div class="file-icon-restricted-block" jsf:rendered="#{fileMetadata.dataFile.notaryServiceBound and fileDownloadHelper.canDownloadFile(fileMetadata)}">
<span title="Notary Service Bound" class="glyphicon glyphicon-tower text-danger"/>

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

This English title should go in a bundle file so it can be translated.

.add("fileOrder", dv.getFileOrder())
.add("UNF",dv.getUnf())
.add("summaryStatistics", org.apache.commons.collections.CollectionUtils.isNotEmpty(dv.getSummaryStatistics()) ? JsonPrinter.jsonSumStat(dv.getSummaryStatistics()) : null)
.add("variableCategories", org.apache.commons.collections.CollectionUtils.isNotEmpty(dv.getCategories()) ? JsonPrinter.jsonCatStat(dv.getCategories()) : null)

This comment has been minimized.

Copy link
@pdurbin

pdurbin Aug 7, 2019

Member

It's interesting that DataVariable isn't already available via API. This seems like a nice addition.

@pdurbin pdurbin moved this from Code Review 🦁 to Community Dev 💻❤️ in IQSS/dataverse Aug 7, 2019

@pdurbin pdurbin assigned akio-sone and unassigned pdurbin Aug 7, 2019

@dataversebot

This comment has been minimized.

Copy link

commented Aug 15, 2019

Can one of the admins verify this patch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.