Create new Item Version (basic Items only) #1318

davide-negretti · 2021-09-16T14:33:39Z

References

Fixes Create new Item Version (basic Items only) #776
Fixes Create a new item version as submitter #780
Requires Item Versioning DSpace#3371

Description

This feature allows to manage item versions. A new version can be created from item's page, and all versions can be managed (create, delete, edit summary) from the "Version History" tab in "Edit Item" page.

Instructions for Reviewers

List of changes in this PR:

Item's public page

A Create new version button has be added (when the user is the submitter or an administrator)

Version's page

A version page /items/version/:versionID has been added
This page redirects to the corresponding item's page

Version History table

The Item column (containing the item's handler and the link to the item page) has been removed
The Version column now contains the link to the version's page (redirecting to the corresponding item's page)
An Action column has been added, containing the buttons to manage the version
The Action column is only displayed in Edit Item page

Authorization features

All actions on versions require authorization
The authorization features are:
- canCreateVersion
- canEditVersion
- canDeleteVersion

Data services

New methods have been added to the following services:
- VersionHistoryDataService

Tests

All tests are still TBD

Checklist

My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
My PR passes TSLint validation using yarn run lint
My PR doesn't introduce circular dependencies
My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
If my PR includes new, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

…laced with modal, minor fixes

…) and modal

…story (call to REST service TBD) and other fixes

…eanup, added modal for new versions

…ge and other fixes

… item's page)

…not working

lgtm-com · 2021-09-16T14:40:49Z

This pull request introduces 2 alerts when merging cf1c73b into 706cc47 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

…ue fixed

lgtm-com · 2021-09-16T17:01:25Z

This pull request introduces 2 alerts when merging b1b2bd4 into 706cc47 - view on LGTM.com

new alerts:

2 for Unused variable, import, function or class

artlowel

That is a big improvement in usability. Thanks @davidenegretti-4science!

I think this can be merged as soon as the test issues are fixed

tdonohue · 2021-10-18T14:43:37Z

@davidenegretti-4science : I'll give this a test today as well. But, I wanted to highlight that this PR is currently failing code coverage, as the coverage drops by -0.54%.... This is a sign that you have too few tests, and that there likely are entire classes with no tests. If you can add more tests to this PR, that'll help get the code coverage back up to where it needs to be to pass.

UPDATE: After glancing at the code, it looks to me like nearly all your new service methods don't have corresponding tests. Adding those should bring code coverage back up. You don't need 100% coverage, but currently you are only at 37% in this PR, and you should be more at 70% at least.

davide-negretti · 2021-10-18T16:17:43Z

@tdonohue I managed to make the coverage test pass but I won't be able to add more tests today, I can increase the coverage tomorrow

…ata.service

tdonohue

@davidenegretti-4science and @atarix83 : I tested this today, and it works well.

That said, there are a lot of missing specs here. I see you both improved coverage a bit (this PR now has ~45% coverage... see the "codecov/patch" check results). However, the rest of main has 75% coverage...so, once we merge this, we'll decrease coverage slightly.

While the decrease in overall coverage isn't significant, I'd still like to see this PR get it's coverage increased into more like a 70% range....especially since I see a lot of new methods which are missing specs in this PR. I've called them out via inline comments below. (A lot of these methods are key functionality in this PR, so they really need specs.)

Overall, I'm +1 this PR. But, I'd like specs added, as they'll help us maintain this code better. Once specs are added, this can be immediately merged. Thanks!

tdonohue · 2021-10-18T19:44:42Z

src/app/core/data/item-data.service.ts

+   * Invalidate the cache of the item
+   * @param itemUUID
+   */
+  invalidateItemCache(itemUUID: string) {


New method missing specs

tdonohue · 2021-10-18T19:45:36Z

src/app/core/data/version-history-data.service.ts

+   * @param itemHref the item for which create a new version
+   * @param summary the summary of the new version
+   */
+  createVersion(itemHref: string, summary: string): Observable<RemoteData<Version>> {


All the changes in this service are missing specs. So, there's around 8 new methods here that have no tests.

tdonohue · 2021-10-18T19:50:16Z

src/app/item-page/simple/item-types/versioned-item/versioned-item.component.ts

+  /**
+   * Open a modal that allows to create a new version starting from the specified item, with optional summary
+   */
+  onCreateNewVersion(): void {


This new method appears to be missing specs

Fixed (specs added)

src/app/shared/item/item-versions/item-versions-shared.service.ts

tdonohue · 2021-10-18T19:54:25Z

src/app/shared/item/item-versions/item-versions.component.ts

+  /**
+   * Applies changes to version currently being edited
+   */
+  onSummarySubmit() {


New method is missing specs

Fixed (specs added)

tdonohue · 2021-10-18T19:54:38Z

src/app/shared/item/item-versions/item-versions.component.ts

+   * Delete the item and get the result of the operation
+   * @param item
+   */
+  deleteItemAndGetResult$(item: Item): Observable<boolean> {


New method is missing specs

tdonohue · 2021-10-18T19:54:54Z

src/app/shared/item/item-versions/item-versions.component.ts

+   * @param version the version to be deleted
+   * @param redirectToLatest force the redirect to the latest version in the history
+   */
+  deleteVersion(version: Version, redirectToLatest: boolean): void {


New method is missing specs

tdonohue · 2021-10-18T19:55:16Z

src/app/shared/item/item-versions/item-versions.component.ts

+   * Creates a new version starting from the specified one
+   * @param version the version from which a new one will be created
+   */
+  createNewVersion(version: Version) {


New method is missing specs

…-shared.service

tdonohue · 2021-10-19T14:15:37Z

@davidenegretti-4science or @atarix83 : Please let me know when this is ready for re-review (in terms of spec updates). As a sidenote, this now has a minor merge conflict after I merged #1327

# Conflicts: # src/app/item-page/item-page-routing.module.ts

tdonohue

@davidenegretti-4science : Thanks for adding more tests. It's now at a more reasonable 55% test coverage (which isn't 70% but it's better). So, I'm going to approve this and merge as we don't have time to wait for more tests. Please, in the future, do your best to add tests as you go & not wait until all code is completed to go back and add your tests/specs.

That all said, honestly, thank you immensely for your hard work on this new feature. I think the user experience is great & it all works well.

davide-negretti · 2021-10-20T20:45:48Z

Thank you @tdonohue
I'm sorry I didn't manage to further increase test coverage in time, however I've been working on a couple of new tests today and I can commit them for a future release.

DSC-983 counters component main

alemarte and others added 18 commits September 2, 2021 16:20

[CST-4499] Version history module (WIP)

5b036e8

[CST-4499] Version history (WIP) - Buttons added to history, form rep…

a1e3124

…laced with modal, minor fixes

[CST-4499] Version history (WIP) - Removed summary page (with routing…

6f24b31

…) and modal

[CST-4499] Version history (WIP) - Added in-row editing in version hi…

95ffc7a

…story (call to REST service TBD) and other fixes

[CST-4499] Version history (WIP) - Changes to in-row editing, code cl…

72bfab1

…eanup, added modal for new versions

[CST-4499] Version history (WIP) - New version modal and other changes

d402ab7

[CST-4499] Version history (WIP) - Fixes to API calls

05385f3

[CST-4499] Version history (WIP) - Fixes to API calls

dd2df91

Changes by alex

ad5ace7

[CST-4499] Version history (WIP) - 'Create version' button in item pa…

44d3558

…ge and other fixes

[CST-4499] Version history (WIP) - Missing tests and auth features

ce399cb

[CST-4499] Version history (WIP) - Version page added (redirecting to…

b4111fe

… item's page)

Merge remote-tracking branch 'bb/CST-4499' into CST-4499

f10f25d

[CST-4499] Version history (WIP) - Refactoring

5d8f625

[CST-4499] Version history (WIP) - Redirect after deleting a version

60bbb9a

[CST-4499] Version history (WIP) - Minor fixes and code cleanup

77ff774

[CST-4499] Version history (WIP) - Authorization features - Deleting …

f858eee

…not working

[CST-4499] Version history (WIP) - Authorization features - Deleting

cf1c73b

atarix83 changed the title ~~Cst 4499~~ Create new Item Version (basic Items only) Sep 16, 2021

tdonohue added this to the 7.1 milestone Sep 16, 2021

tdonohue added the work in progress label Sep 16, 2021

tdonohue requested review from tdonohue and artlowel September 16, 2021 14:52

tdonohue added the component: versioning label Sep 16, 2021

[CST-4499] Version history (WIP) - Authorization features - Cache iss…

b1b2bd4

…ue fixed

[CST-4499] Version history (WIP) - Cache issue fixed

025e7f5

artlowel approved these changes Oct 18, 2021

View reviewed changes

[CST-4499] Version history - Test

bee6289

atarix83 added 2 commits October 18, 2021 19:04

[CST-4499] Add tests to workspaceitem-data.service and workflowitem-d…

8f541c3

…ata.service

[CST-4499] Add tests to version-data.service

e142a49

tdonohue requested changes Oct 18, 2021

View reviewed changes

[CST-4499] Version history - Test item-data.service and item-versions…

376d803

…-shared.service

github-actions bot added the merge conflict label Oct 19, 2021

Davide Negretti added 6 commits October 19, 2021 16:18

Merge branch 'main' of github.com:4science/dspace-angular into CST-4499

6dac829

[CST-4499] Version history - Test version-history-data.service

3cc302b

Merge branch 'main' into CST-4499

82f6a78

# Conflicts: # src/app/item-page/item-page-routing.module.ts

[CST-4499] Version history - Test version-history-data.service

75fffe2

[CST-4499] Version history - Test version-history-data.service

d12f932

[CST-4499] Version history - Test versioned-item.component

52baa81

tdonohue removed the merge conflict label Oct 19, 2021

Davide Negretti added 2 commits October 20, 2021 02:28

[CST-4499] Version history - Test item-versions.component

3b0e1db

[CST-4499] Version history - Test item-versions.component

fd61ac0

tdonohue approved these changes Oct 20, 2021

View reviewed changes

tdonohue merged commit 2568dae into DSpace:main Oct 20, 2021

This was referenced Oct 20, 2021

Angular support for IIIF and Mirador #1079

Merged

Assign an entity type to a collection and map external providers #1332

Merged

abollini deleted the CST-4499 branch October 26, 2021 07:22

4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Feb 6, 2024

Merged in DSC-983-counters-component-main (pull request DSpace#1318)

c1c5a66

DSC-983 counters component main

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create new Item Version (basic Items only) #1318

Create new Item Version (basic Items only) #1318

davide-negretti commented Sep 16, 2021 •

edited

Loading

lgtm-com bot commented Sep 16, 2021

lgtm-com bot commented Sep 16, 2021

artlowel left a comment

tdonohue commented Oct 18, 2021 •

edited

Loading

davide-negretti commented Oct 18, 2021

tdonohue left a comment •

edited

Loading

tdonohue Oct 18, 2021

tdonohue Oct 18, 2021

tdonohue Oct 18, 2021

davide-negretti Oct 19, 2021

tdonohue Oct 18, 2021

davide-negretti Oct 20, 2021

tdonohue Oct 18, 2021

tdonohue Oct 18, 2021

tdonohue Oct 18, 2021

tdonohue commented Oct 19, 2021

tdonohue left a comment

davide-negretti commented Oct 20, 2021

Create new Item Version (basic Items only) #1318

Create new Item Version (basic Items only) #1318

Conversation

davide-negretti commented Sep 16, 2021 • edited Loading

References

Description

Instructions for Reviewers

List of changes in this PR:

Item's public page

Version's page

Version History table

Authorization features

Data services

Tests

Checklist

lgtm-com bot commented Sep 16, 2021

lgtm-com bot commented Sep 16, 2021

artlowel left a comment

Choose a reason for hiding this comment

tdonohue commented Oct 18, 2021 • edited Loading

davide-negretti commented Oct 18, 2021

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue commented Oct 19, 2021

tdonohue left a comment

Choose a reason for hiding this comment

davide-negretti commented Oct 20, 2021

davide-negretti commented Sep 16, 2021 •

edited

Loading

tdonohue commented Oct 18, 2021 •

edited

Loading

tdonohue left a comment •

edited

Loading