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

Internationalization - Terms of Use #5303

Closed
JayanthyChengan opened this issue Nov 9, 2018 · 10 comments
Closed

Internationalization - Terms of Use #5303

JayanthyChengan opened this issue Nov 9, 2018 · 10 comments
Assignees
Milestone

Comments

@JayanthyChengan
Copy link
Contributor

Requirement for internaionalization:
terms of use -

image

@pdurbin
Copy link
Member

pdurbin commented Nov 13, 2018

@JayanthyChengan thanks for pull request #5304

I took a quick look and the first thing that struck me is that you are changing how "General Terms of Use" is configured in the application. We've always configured it with the ":ApplicationTermsOfUse" database setting like this:

screen shot 2018-11-13 at 12 04 00 pm

Thanks from http://guides.dataverse.org/en/4.9.4/installation/config.html#applicationtermsofuse and I believe #2669 was recently fixed by pull request #5269, by the way.

Please edit doc/sphinx-guides/source/installation/config.rst in your pull request to remove the old way and add the new way to set General Terms of Use.

Also, while you're on that branch, please take a look at the preferred placement of curly braces at http://guides.dataverse.org/en/4.9.4/developers/coding-style.html#braces-placement . In Java it's valid to leave out the braces in the following code...

    if(appTermsOfUse != null)
        return appTermsOfUse;
    else
        return saneDefaultForAppTermsOfUse;

... but we prefer to keep them in like this:

    if (appTermsOfUse != null) {
        return appTermsOfUse;
    } else {
        return saneDefaultForAppTermsOfUse;
    }

If you reformat the lines you changed as advised at http://guides.dataverse.org/en/4.9.4/developers/coding-style.html#format-code-you-changed-with-netbeans Netbeans will add the curly braces in for you automatically.

@JayanthyChengan
Copy link
Contributor Author

@pdurbin - updated the docs. I noticed that in this approach, by default, any new dataverse installation will display the "Dataverse General Terms of Use" from termsofuse.properties instead of "There are no Terms of Use for this Dataverse installation". Is that ok? , if not , how can we design?

@pdurbin
Copy link
Member

pdurbin commented Nov 13, 2018

@JayanthyChengan thanks. I think this line in the guide is a little weird in that it shows what we've started calling "flagship bias":

By default, the Sign Up page will show the “Harvard Dataverse General Terms of Use”.

Here's a link to our "flagship bias" doc: https://docs.google.com/document/d/1-LtDv_Yaf_3EPuMWcg5ZxH7M2ACrmCzPzgmbwbt8gMQ/edit?usp=sharing

I don't link highlighting the flagship installation of Dataverse (Harvard Dataverse) in our guides. I'd rather have the out-of-the-box configuration show "not configured" or something. But, I don't want to hold up progress so I'm moving this to QA to at least get some other opinions. Thanks for documenting the change!

@kcondon kcondon self-assigned this Nov 14, 2018
pdurbin added a commit to scholarsportal/dataverse that referenced this issue Nov 14, 2018
@pdurbin
Copy link
Member

pdurbin commented Nov 14, 2018

@kcondon and I discussed this issue after standup and agreed that the out-of-the-box settings should be the same. That is to say, "General Terms of Use" on the Sign Up page should continue to show "There are no Terms of Use for this Dataverse installation." I attempted to implement this in f09e897 but heads up to @JayanthyChengan that I had trouble following the docs and ended up fixing what I assume is a typo. The files should go in the path for "dataverse.lang.directory", right?

@kcondon something else that occurs to me is that we should add a file to docs/release-notes (#5284) once we're sure the code is working as intended. Unless installations put their General Terms of Use in the new location (on the filesystem instead of in the database) they won't be displayed.

@JayanthyChengan
Copy link
Contributor Author

sorry - it's typo and Yes, once the JVM option for "dataverse.lang.directory" is specified, all the property files should be saved under it.

Thanks to all.

@scolapasta
Copy link
Contributor

@JayanthyChengan @pdurbin @kcondon @djbrooke
I'm a little torn on this one.

On the one hand, moving the terms of use out of the setting makes sense since it's displayed text and generally displayed text is on the bundles.

On the other hand, all the other displayed text is not really text that an installation configures. Yes, an installation choses their language selections and metadata blocks, but once they do, they should be able to use the out of the box bundles and not have to change them.

In other words, the text that we have in the bundles isn't really user input data. The terms of use ae user input, where the user is a installation admin.

Additionally if this were a column in the db, such that to add a new language we would need to add a new column (i.e. change the db structure), that would definitely be not scalable. But in the case of terms of use, adding a new language could be accomplished by adding a new row to the setting table (imagine one setting per language). This is doable by API. Changing a bundle is not.

Thoughts from others?

@scolapasta
Copy link
Contributor

So after mulling over some more (and discussing this at standup), it's clear that this would be better served by still being in the db and not the bundles. So we can still use the settings table and there would be a setting per locale (it make sense to have a column for the locale that will be optional (since most settings won't require it). Additionally the default value can have no locale.

There should then be helper methods that get you the right value from the table based on the locale (and if it doesn't return a value uses the default).

@JayanthyChengan let me know if this makes sense to you and if not, if you need further clarification.

@JayanthyChengan
Copy link
Contributor Author

@scolapasta I will follow the above mentioned design and submit it soon.

@pdurbin
Copy link
Member

pdurbin commented Mar 20, 2019

@JayanthyChengan head up that pull request #5304 has merge conflicts.

@JayanthyChengan
Copy link
Contributor Author

JayanthyChengan commented Jul 19, 2019

@scolapasta @pdurbin - as mentioned in above comment(link), I added new column(lang) to the setting table.

Now Setting table got three columns (name, content, lang) and "name" as primary key

When I add new row (:ApplicationTermsOfUse , "...." , "fr" ) using curl command, I get the below exception,

org.postgresql.util.PSQLException: ERROR: duplicate key value violates unique constraint "setting_pkey"

So, modified primary key

ALTER TABLE ONLY setting
ADD CONSTRAINT setting_pkey PRIMARY KEY (name,lang);

Please review the PR #6042

curl command to delete Database Setting with lang option:

curl http://localhost:8080/api/admin/settings/:ApplicationTermsOfUse/lang/fr -X DELETE

curl command to add Database Setting with lang option:
curl -X PUT -d@/tmp/apptou.html 
http://localhost:8080/api/admin/settings/:ApplicationTermsOfUse/lang/fr

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants