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.

Sign up for GitHub

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

Jump to bottom

[VIVO-1436] Implementation of Advanced Role Management #85

Closed
wants to merge 16 commits into from
Closed

[VIVO-1436] Implementation of Advanced Role Management #85

wants to merge 16 commits into from

Conversation

grahamtriggs
Copy link
Member

Reopening as old pull request was incorrect

See vivo-project/Vitro#80 for details

@j2blake j2blake self-requested a review October 23, 2018 15:13
j2blake
j2blake suggested changes Oct 23, 2018
View reviewed changes
Copy link
Member

@j2blake j2blake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

home/src/main/resources/rdf/display/firsttime/PropertyConfig.n3 and
home/src/main/resources/rdf/tbox/firsttime/vitroAnnotations.n3
still contain hundreds of references to
http://vitro.mannlib.cornell.edu/ns/vitro/0.7#hiddenFromDisplayBelowRoleLevelAnnot,
http://vitro.mannlib.cornell.edu/ns/vitro/0.7#prohibitedFromUpdateBelowRoleLevelAnnot, and
http://vitro.mannlib.cornell.edu/ns/vitro/0.7#hiddenFromDisplayBelowRoleLevelAnnot

j2blake
j2blake reviewed Oct 23, 2018
View reviewed changes
Copy link
Member

@j2blake j2blake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my full list of review points, including comments on unused code, commented code, undocumented methods, and others.

Since the project doesn't have a style guide, these are simply observations. Act on them or not, as you see fit.

VIVO - main

edu.cornell.mannlib.vivo.auth.policy.InfoContentEntityChecker
lines 7, 9 Logic imports of PolicyDecision and AbstractPropertyStatementAction are not used.

edu.cornell.mannlib.vivo.auth.policy.SelfEditorRelationship
line 43 Style Why use a nested class?
line 46 Logic This line has no function.

edu.cornell.mannlib.vivo.auth.policy.AdvisingRelationshipChecker
lines 7, 9 Logic These import statements are not used.

edu.cornell.mannlib.vivo.auth.policy.CourseChecker
lines 7, 9 Logic These import statements are not used.

edu.cornell.mannlib.vivo.auth.policy.GrantChecker
lines 7, 9 Logic These import statements are not used.

edu.cornell.mannlib.vivo.auth.policy.PresentationChecker
lines 7,9 Logic These import statements are not used.

edu.cornell.mannlib.vivo.auth.policy.ProjectOrServiceChecker
lines 7, 9 Logic These import statements are not used.

VIVO - home

src/main/resources/rdf/auth/firsttime/permission_entities.n3

OK

VIVO - webapp

src/main/webapp/WEB-INF/resources/startup_listeners.txt

OK

awoods
awoods requested changes Oct 24, 2018
View reviewed changes
api/src/main/java/edu/cornell/mannlib/vivo/auth/policy/SelfEditorRelationship.java Outdated
public static class Setup implements ServletContextListener {
@Override
public void contextInitialized(ServletContextEvent sce) {
ServletContext ctx = sce.getServletContext();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ctx variable does not appear to be used... and can likely be removed.

webapp/src/main/webapp/WEB-INF/resources/startup_listeners.txt Outdated
@@ -33,6 +33,9 @@ edu.cornell.mannlib.vitro.webapp.servlet.setup.FileGraphSetup
# edu.cornell.mannlib.vitro.webapp.migration.rel17.Release17Migrator
edu.cornell.mannlib.vitro.webapp.migration.rel18.Release18Migrator

edu.cornell.mannlib.vitro.webapp.migration.auth.AuthMigrator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment describing this migrator.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

@grahamtriggs
Copy link
Member Author

@j2blake The issues with leftover roles in the ontology files was a deliberate choice - it is relatively easy (albeit slightly time consuming) to delete lines that are no longer required. However, not knowing how long it will take to review the role management, or what might happen in ontology development in that time, I was hoping to reduce potential merge problems by delaying removing the old permissions.

@grahamtriggs
Copy link
Member Author

@j2blake About the nested class in SelfEditorRelationship - it was done for consistency with other setup code that is called from startup_listeners.txt. I've added a comment to the class.

awoods pushed a commit to awoods/Vitro that referenced this pull request Nov 20, 2018
Address code review comments
64b0a8c 
    From:
    - Vitro: vivo-project#80
    - VIVO: vivo-project/VIVO#85

    Resolves:
    https://github.com/vivo-project/Vitro/pull/80/files#r227428235
    https://github.com/vivo-project/Vitro/pull/80/files#r227892906
    https://github.com/vivo-project/Vitro/pull/80/files#r227905400
    https://github.com/vivo-project/Vitro/pull/80/files#r227907884
    https://github.com/vivo-project/Vitro/pull/80/files#r227909775
    https://github.com/vivo-project/Vitro/pull/80/files#r227913953
    https://github.com/vivo-project/Vitro/pull/80/files#r227913145
    https://github.com/vivo-project/Vitro/pull/80/files#r227913209
    https://github.com/vivo-project/Vitro/pull/80/files#r227914673
    https://github.com/vivo-project/Vitro/pull/80/files#r227915444
    https://github.com/vivo-project/Vitro/pull/80/files#r227960329

    Not addresses:
    https://github.com/vivo-project/Vitro/pull/80/files#r227429141
    https://github.com/vivo-project/Vitro/pull/80/files#r227434076
awoods pushed a commit to awoods/VIVO that referenced this pull request Nov 20, 2018
Address code review comments
f5b6a27 
From:
- Vitro: vivo-project/Vitro#80
- VIVO: vivo-project#85

Resolves:
https://github.com/vivo-project/Vitro/pull/80/files#r227428235
https://github.com/vivo-project/Vitro/pull/80/files#r227892906
https://github.com/vivo-project/Vitro/pull/80/files#r227905400
https://github.com/vivo-project/Vitro/pull/80/files#r227907884
https://github.com/vivo-project/Vitro/pull/80/files#r227909775
https://github.com/vivo-project/Vitro/pull/80/files#r227913953
https://github.com/vivo-project/Vitro/pull/80/files#r227913145
https://github.com/vivo-project/Vitro/pull/80/files#r227913209
https://github.com/vivo-project/Vitro/pull/80/files#r227914673
https://github.com/vivo-project/Vitro/pull/80/files#r227915444
https://github.com/vivo-project/Vitro/pull/80/files#r227960329

Not addresses:
https://github.com/vivo-project/Vitro/pull/80/files#r227429141
https://github.com/vivo-project/Vitro/pull/80/files#r227434076
@awoods awoods mentioned this pull request Nov 20, 2018
Closed
@grahamtriggs grahamtriggs changed the base branch from develop to dev-ctsaconnect June 27, 2019 21:45
@grahamtriggs grahamtriggs changed the base branch from dev-ctsaconnect to develop June 27, 2019 21:45
@awoods awoods closed this Jan 28, 2020
@gneissone
Copy link
Member

Reopening this pull request. In an effort to follow current best practices, the VIVO project will now be developed against the master branch. As a result of removing the develop branch, this pull request was automatically closed by GitHub, apologies for any confusion this may have caused.

@gneissone gneissone reopened this Feb 5, 2020
@gneissone gneissone changed the base branch from develop to master February 5, 2020 16:22
Base automatically changed from master to main January 20, 2021 17:28
awoods and others added 4 commits February 5, 2021 10:56
@awoods @wwelling @lb-ov
Enable war deployment model (vivo-project#197)
01776ee 
* Add 'home' files into build war artifact

Related to: https://jira.lyrasis.org/browse/VIVO-1443

* Disable copying exploded war to Tomcat dir

- hardcode vivo.all.log file name
- must now set system property: -Dvivo-dir=/opt/vivo/home/

Related to: https://jira.lyrasis.org/browse/VIVO-1443

* Ensure build does not remove and re-add VIVO_HOME/rdf

Related to: https://jira.lyrasis.org/browse/VIVO-1443

* Remove unnecessary profile from installer/pom.xml

Related to: https://jira.lyrasis.org/browse/VIVO-1443

* Rename example config files to have 'default' prefix

Related to: https://jira.lyrasis.org/browse/VIVO-1443

* Require common properties to be in JNDI

Properties include:
- vitro/home
- vitro/appName
- vitro/rootUserAddress
- vitro/defaultNamespace

Related to: https://jira.lyrasis.org/browse/VIVO-1443

* VIVO-1443: app name (#2)

* Non-functional change to comment in example.applicationSetup.n3

Related to: https://jira.lyrasis.org/browse/VIVO-1741

* Update orcidConfirm.ftl (vivo-project#199)

* remove example-settings.xml
* simplify war name and afford override during build
* default app name to vivo and pass into context.xml

Co-authored-by: Andrew Woods <[email protected]>
Co-authored-by: L.O <[email protected]>

Co-authored-by: Andrew Woods <[email protected]>
Co-authored-by: William Welling <[email protected]>
Co-authored-by: L.O <[email protected]>
@grahamtriggs
Implementation of Advanced Role Management, including migration of ex…
b701a85 
…isting roles
@grahamtriggs
Improvements requested in PR review
167e33d
@grahamtriggs
Improvements requested in PR review
ec6fc8f
@chenejac chenejac mentioned this pull request Oct 14, 2021
Closed
@litvinovg
Copy link
Collaborator

Superseded by #3887

@litvinovg litvinovg closed this Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j2blake j2blake j2blake requested changes

@awoods awoods awoods requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grahamtriggs @gneissone @litvinovg @awoods @j2blake