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

DS-3699 expose submission configuration over REST #1852

Merged
merged 54 commits into from Nov 16, 2017
Merged

Conversation

@abollini
Copy link
Member

abollini commented Sep 28, 2017

This is the implementation of the REST endpoints needed to expose the submission configuration as described in the REST Contract PR: DSpace/Rest7Contract#11

It is based on the work in the PR #556

Up to now only the submission configs (aka submission definitions) are exposed, I will add soon the commits related to the input-forms

@abollini abollini changed the title DS-3699 DS-3699 expose submission configuration over REST Sep 30, 2017
@abollini abollini force-pushed the 4Science:DS-3699 branch from 9697412 to e6e9216 Oct 1, 2017
abollini and others added 25 commits Sep 28, 2017
…ic discovery of valuepairs and vocabulary as authority; exposition of relid; D4CRIS-342 improve selectablemetadata; add check on inputform converter to add authority only if authority have choice;
Copy link
Contributor

tomdesair left a comment

I've done a second pass with some more in-depth questions. But since this is a big chuck of code, it would be good if others could also take a look.

}
}
return results;
} catch (ServletException e) {

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

👍

// (b) this is a workflow process, and this step is editable in a
// workflow
if ((!this.isWorkflow)
|| ((this.isWorkflow) && step.isWorkflowEditable()))

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

Just to be sure: We remove the workflow check here in the backend because this check should happen in the front-end instead?

This comment has been minimized.

Copy link
@lap82

lap82 Oct 31, 2017

Contributor

No, please take in mind that this PR it's only the first stage, to move forward quickly.
A new issue will be the target to works togheter and define how to expone workflow submission for validators.

This comment has been minimized.

Copy link
@tomdesair

tomdesair Nov 2, 2017

Contributor

Ok, thanks for the clarification.

@@ -562,6 +593,12 @@ else if (submitNames.contains(submitName))
stepInfo.put("id", stepID);
}

String mandatory = getAttribute(nStep, "mandatory");
if (mandatory != null && mandatory.length() > 0)

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

Minor comment: you can use StringUtils.isNotBlank here

/**
* The scope restriction for this step (submission or workflow).
**/
private String scope = null;

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

To make the meaning of scope more clear. Can you change this to an enum with values SUBMISSION, WORKFLOW, VERSIONING ?

This comment has been minimized.

Copy link
@lap82

lap82 Oct 31, 2017

Contributor

IMHO it is early to introduce enumeration. In the second stage we will introduce enumeration if needed, now the contract saying nothing (see https://github.com/DSpace/Rest7Contract/blob/master/submissionsections.md) and I think that from the item-submission.dtd the values can be other than the tree listed by you.

This comment has been minimized.

Copy link
@tomdesair

tomdesair Nov 2, 2017

Contributor

But since you have an enum for this in the REST model, wouldn't it make sense to also have a similar enum in the domain model?

}
for (int i = 0; i < inputs.length; i++) {
String inputField = inputs[i].getSchema() + "." + inputs[i].getElement()
+ (inputs[i].getQualifier() == null ? "" : "." + inputs[i].getQualifier());

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

👍

* Locale, the local to get the input-forms.xml for
* @return String - localized filename for input-forms.xml
* Locale, the local to get the submission-forms.xml for
* @return String - localized filename for submission-forms.xml
*/
public static String getInputFormsFileName(Locale locale)

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

Do we still need this method? Won't all i18n related stuff be done by the UI?

This comment has been minimized.

Copy link
@lap82

lap82 Oct 31, 2017

Contributor

This will be done in the second stage. Thank you!


protected String makeFieldKey(String schema, String element, String qualifier)
{
if (qualifier == null)
if (StringUtils.isBlank(qualifier))
{
return schema + "_" + element;

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

Can't you reuse the Utils.tokenize method here which you introduced?

{
throw new ServletException(e);
}
return 1;

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

Then I would remove the getNumberOfPages method to avoid confusion.

SelectableMetadata selMd = new SelectableMetadata();
if (authorityUtils.isChoice(dcinput.getSchema(), dcinput.getElement(), dcinput.getQualifier())) {
inputRest.setType(
getPresentation(dcinput.getSchema(), dcinput.getElement(), dcinput.getQualifier(), inputType));

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

Just a question: Shouldn't authority presentation information be retrieved from the corresponding authority endpoint?

This comment has been minimized.

Copy link
@lap82

lap82 Oct 31, 2017

Contributor

It is a concept related to metadata type, that in certain case have to modify the presentation. The authority endpoint is not presentation aware because expone only information related the structure and the data.


/**
* The SelectableMetadata REST Resource. It is not addressable directly, only
* used as inline object in the InputForm resource

This comment has been minimized.

Copy link
@tomdesair

tomdesair Oct 27, 2017

Contributor

These are the same values that can be retrieved from the authority endpoint, correct? Do we have to include them in the InputForm resource?

This comment has been minimized.

Copy link
@lap82

lap82 Oct 31, 2017

Contributor

SelectableMetadata is an inner resource in the inputform resource, it is different from the values retrieved into authority endpoint.

This comment has been minimized.

Copy link
@tomdesair

tomdesair Nov 2, 2017

Contributor

I thought every value-pair was also auto-registered as an authority now? Can you give an example of a use case where this doesn't link to a value-pair?

This comment has been minimized.

Copy link
@abollini

abollini Nov 2, 2017

Author Member

SelectableMetadata was introduced to make a clear distinction between the cases where a value-pairs was used as an authority list of acceptable values (dropdown) and where it was used to allow to pick the metadata to use to store the value (qualdrop_values).
If a value-pair is used by a qualdrop_value it is not autoregistered as an authority see https://github.com/DSpace/DSpace/pull/1852/files/779f561e6890c9e83e70456213fa0ced41f1fb07#diff-3b9150885fcc1931b62efc9ddd18c9aeR292 it is exposed as an array of SelectableMetadata object

This comment has been minimized.

Copy link
@tomdesair

tomdesair Nov 8, 2017

Contributor

Ok that clarifies it. But I would add that to the comments of this class.

lap82 and others added 3 commits Oct 31, 2017
…es class field; replace logic into the makefield method in favour of Utils.standardize static method;
}
} // ignore any child that is not a 'page'
Node npg = pl.item(j);
// process each page definition

This comment has been minimized.

Copy link
@tdonohue

tdonohue Nov 1, 2017

Member

remove this comment...it's duplicated below

// we omit the duplicate validation, allowing multiple
// fields definition for
// the same metadata and different visibility/type-bind
}
}
// sanity check number of pages

This comment has been minimized.

Copy link
@tdonohue

tdonohue Nov 1, 2017

Member

should say "fields" not "pages"

// Tokens contains:
// schema = tokens[0];
// element = tokens[1];
// qualifier = tokens[2];

This comment has been minimized.

Copy link
@tdonohue

tdonohue Nov 1, 2017

Member

These comments should probably be in the Javadocs for this method. That way they are easily found.

Copy link
Member

tdonohue left a comment

👍 This looks good overall. A few comments inline.

I would like to see a rough draft of documentation comparing the old configs to the new ones (even just a wiki page). There's a lot of code changes here, and it's a bit hard to get your mind around it all without docs.

DS-3724: Missing readonly in input-forms.dtd
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 2, 2017

Coverage Status

Changes Unknown when pulling f755f59 on 4Science:DS-3699 into ** on DSpace:master**.

@tomdesair

This comment has been minimized.

Copy link
Contributor

tomdesair commented Nov 2, 2017

I've added some replies. My major concern is still with InputFormSelfRegisterWrapperAuthority and how it relates to DCInputAuthority and DSpaceControlledVocabulary. Maybe this can be refactored or clarified in the documentation.

@abollini

This comment has been minimized.

Copy link
Member Author

abollini commented Nov 2, 2017

@tomdesair see my inline comments, in particular #1852 (diff) and the https://jira.duraspace.org/browse/DS-3750

@tdonohue I have created this issue https://jira.duraspace.org/browse/DS-3744 and sub-tasks to track the configuration changes. The ongoing changes to the input-forms and item-submission are also documented on this wiki page: https://wiki.duraspace.org/display/DSPACE/Configuration+changes+in+the+submission+process

@lap82

This comment has been minimized.

Copy link
Contributor

lap82 commented Nov 3, 2017

@tdonohue I think that with the last changes this PR is ready to merge. We have to take in mind that this is only the first stage that have as primary goal the exposition of submission definition and its input forms via rest endpoint. In the future as reported by @abollini there will be other changes to modernize this infrustructure. We thinking to introduce item-submission.xml and input-forms.xml as spring bean or the use of Spring IOC to manage ChoiceAuthority (this will permits to rearrange the two "legacy" authority plugin - DCInputAuthority and DSpaceControlledVocabulary).

@lap82

This comment has been minimized.

Copy link
Contributor

lap82 commented Nov 3, 2017

UPDATE: I'm working to fix the new integration test

DS-3484 catch generic Exception
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 3, 2017

Coverage Status

Coverage increased (+0.2%) to 21.049% when pulling 216d748 on 4Science:DS-3699 into 716912f on DSpace:master.

Copy link
Member

tdonohue left a comment

👍 Looks good. It looks to me like all requested changes have either been made or created as separate tickets (to be addressed in future).

Copy link
Contributor

tomdesair left a comment

The changes that have been made are good for a first iteration of this endpoint. But I still have some open requests:

  • Update the comments of SelectableMetadata.java to explain the qualdrop_values use case (#1852 (comment))
  • There are still some ServletException catch clauses that we might want to clean up.
  • The REST model has an enum for the scope field while the domain model has not (see #1852 (comment)). I think we should align this.
  • The dspace/modules/spring-rest/src/main/webapp/.gitignore file is empty and should be removed
  • I really think we need integration tests for this endpoint which validate the REST contract structure and default config values, so that we safely refactor and improve this code afterwards. We need to make sure to not increase the testing backlog we already have:
    • SubmissionDefinitionsControllerIT that should test
      • /api/config/submissiondefinitions
      • /api/config/submissiondefinitions/<:definition-name>
      • /api/config/submissiondefinitions/search/findByCollection?uuid=<:collection-uuid>
      • /api/config/submissiondefinitions/<:definition-name>/collections
      • /api/config/submissiondefinitions/<:definition-name>/sections
    • SubmissionFormsControllerIT that should test
      • /api/config/submissionforms
      • /api/config/submissionforms/<:form-name>
    • SubmissionSectionsControllerIT that should test
      • /api/config/submissionsections
@abollini

This comment has been minimized.

Copy link
Member Author

abollini commented Nov 8, 2017

I have applied the first two requests from @tomdesair

Update the comments of SelectableMetadata.java to explain the qualdrop_values use case (#1852 (comment))
There are still some ServletException catch clauses that we might want to clean up.

I would prefer to postpone the porting of the Enum to the dspace-api as I think we need a more wide refactoring on the SubmissionConfigReader and DCInputsReader to make them more consistent with each other that in turn will require to modify the .xml files... since we are probably going to replace this files with spring beans it could be a waste of time to work on that now. We can just resume that later (I have created the https://jira.duraspace.org/browse/DS-3754 to avoid to forget about that)

I ask @lap82 if we can remove the .gitignore file noted by @tomdesair , I don't remember if it was introduced to make sure that the folder is here with a clean checkout of the project for a purpose or not...

I agree about the need of the tests but we are still in the phase where more endpoints is better than stable endpoints in my opinion... if we wait for the test we will have a couple of PRs that will be impossible to review as they will be based on this one. So here https://jira.duraspace.org/browse/DS-3755 the issue to remember about the tests, we can put a deadline for the end of the year on that but not more. @tdonohue @tomdesair What do you think?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 8, 2017

Coverage Status

Coverage increased (+0.2%) to 21.043% when pulling 6b63b6e on 4Science:DS-3699 into 716912f on DSpace:master.

Copy link
Contributor

tomdesair left a comment

Thanks for the changes @abollini. I'll sleep better now that we have a ticket to remind of us the missing tests for this endpoint and to align both models. 😉

This is OK for me now.

@lap82

This comment has been minimized.

Copy link
Contributor

lap82 commented Nov 16, 2017

I ask @lap82 if we can remove the .gitignore file noted by @tomdesair , I don't remember if it was introduced to make sure that the folder is here with a clean checkout of the project for a purpose or not...

the file .gitignore in the path /modules/[name-module]/src/main/webapp/ was introduced by @peterdietz six years old ago to track empty folders with git. There is alternative convention if the use of .gitignore confusing, but now I suggest to mantain it as is.

@abollini abollini merged commit 00ba22e into DSpace:master Nov 16, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 21.043%
Details
@abollini abollini deleted the 4Science:DS-3699 branch Nov 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.