...
- A mock of BrowseCreateDAOOracle has been done due to an incompatibility between H2 and the "upper" function call. This will affect tests related to case sensitivity in indexes.
- Many objects (like SupervisedItem) lack a proper definition of the "equals" method, which makes comparison between objects and unit testing harder
- Update method of many objects doesn't provide any feedback, we can only test if it raises an exception or not, but we can't be 100% sure if it worked
- Many objects have methods to change the policies related to the object or children objects (like Item), it would be good to have some methods to retrieve these policies also in the same object (code coherence)
- There are some inconsistencies in the calls to certain methods. For example getName returns an empty String in a Collection where the name is not set, but a null in an Item without name
- DCDate: the tests raise many errors. I can't be sure if it's due to misunderstanding of the purpose of the methods or due to faulty implementation (probably the previous). In some cases extra encapsulation of the internals of the class would be advisable, to hide the complexities of the Calendars (months starting by 0, etc)
- The Authorization system gets a bit confusing. We have AuthorizationManager, AuthorizationUtils, methods that raise exceptions and methods that return booleans. Given the number of checks we have to do for permissions, and that some classes call methods that require extra permissions not declared or visible at first, this makes creation of tests (and usage of the API) a bit complex. I know we can ignore all permissions via context (turning on and off authorizations) but usually we don't want that
- Community: set logo checks for authorization, but set metadata doesn't. It's in purpose?
- Collection: methods create and delete don't check for authorization
- Item: there is no authorization check for changing policies, no need to be an administrator
- ItemIterator: it uses ArrayLists in the methods instead of List
- ItemIterator: we can't verify if the Iterator has been closed
- Site: this class extends DSpaceObject. With it being a Singleton, it creates potential problems, for example when we use DSpaceObject methods to store details in the object. It's this relation necessary?
Fixes done:
- Bitstream: added an "isDeleted" method to verify if a bitstream has been deleted
- Bundle: added methods to check the policies of bundle and its bitstreams
- Collection: just a comment: delete requires authorization to remove the template Item and write, not to remove. Is that correct?
- Community: when a community is created with a parent, it's added as a child community immediately
- DCLanguage: added checks for null name in languages
- FormatIdentifier: fixed the check for filename == null in guessFormat
- SiteTest: the test is in the abstract DSpaceObjectTest so I've make it inherit AbstractUnitTest. I see the class has almost no usage so we could remove the inheritance from DSpaceObject, but I'm not sure if to do this. It's something that we should ask the developers?
- Several equals and hashCode methods added for other issues in tests
Pending of a review by a DSpace developer:
- DCDate: Here many tests fail because I'm not sure of the purpose of the class. I would expect it to hide the implementation of Calendar (with all those things like months starting by 0 and other odd stuff) so it's easier to use, but it seems that's not the case...
- Bitstream: added an "isDeleted" method to verify if a bitstream has been deleted
- Bundle: added methods to check the policies of bundle and its bitstreams
- Collection: just a comment: delete requires authorization to remove the template Item and write, not to remove. Is that correct?
- Community: when a community is created with a parent, it's added as a child community immediately
- DCLanguage: added checks for null name in languages
- FormatIdentifier: fixed the check for filename == null in guessFormat
- SiteTest: the test is in the abstract DSpaceObjectTest so I've make it inherit AbstractUnitTest. I see the class has almost no usage so we could remove the inheritance from DSpaceObject, but I'm not sure if to do this. It's something that we should ask the developers?
- Several equals and hashCode methods added for other issues in tests
Proposals:
To solve the previous issues, some proposals are done:
...