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

Item-Collection mapper #348

Open
wants to merge 109 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Atmire-Kristof
Copy link
Contributor

Atmire-Kristof commented Jan 4, 2019

This feature is based on the “Mapped collections” feature in DSpace6.

collection-item mapper
The route to this page can be found at /collections/<uuid>/mapper. Here, the user (when logged in), has the ability to map items to the current collection.
The page exists out of a description and two tabs labelled “Browse” and “Map”.
The “Browse” tab contains items already mapped to the collection and the option to un-map items by selecting them and pressing the confirm button. The “Map” tab contains all items not yet mapped to or part of the collection and the option to map items by selecting them and pressing the confirm button.
You’ll also notice the “Map” tab has a search feature, as this page will usually contain a lot of items.

item-collection mapper
The route to this page can be found at /items/<uuid>/edit/mapper or by navigating to the edit page and pressing the “Mapped collections” button.
Just like the collection-item mapper, this page contains two tabs, “Browse” and “Map”.
The “Browse” tab contains all collections the item is mapped to. The “Map” tab contains all collections the item is not yet mapped to or part of.
Both also have the option to select collections for un-mapping or mapping the item.

object-select
An abstract object-select component was added, which can be extended depending on the type of object the list is composed of. For this PR, two components exist that extend this one: collection-select and item-select.

input

  • key: This should be a unique key used for storing the selected objects into the store. This way they don’t interfere with one another.
  • dsoRD$: Observable<RemoteData> with TDomain being any type of object defined in the child component
  • paginationOptions: The PaginationComponentOptions used for the list
  • confirmButton: A message key used to display on the confirmation button
  • hideCollection: a boolean used to disable the collection column

output

  • confirm: A list of UUIDs of the selected objects after pressing the confirmation button (string[])

Atmire-Kristof and others added some commits Sep 10, 2018

Merge remote-tracking branch 'atmire-internal/w2p-55565_authorization…
…-issue' into patch-support

Conflicts:
	package.json
	src/app/+search-page/search-filters/search-filters.component.ts
	src/app/core/auth/auth.effects.ts
	src/app/core/auth/auth.service.ts
	src/app/core/auth/server-auth.service.ts
	src/app/core/data/data.service.ts
	src/app/header/header.component.spec.ts
	src/app/shared/auth-nav-menu/auth-nav-menu.component.ts
	src/app/shared/testing/auth-service-stub.ts
	yarn.lock
Merge branch 'master' into w2p-55693_Item-mapping-to-collections
Conflicts:
	src/app/core/auth/auth-response-parsing.service.ts
	src/app/core/auth/auth.service.ts
	src/app/core/auth/models/auth-status.model.ts
	src/app/core/auth/models/normalized-auth-status.model.ts
	src/app/core/auth/server-auth.service.ts
	src/app/shared/testing/auth-request-service-stub.ts
	src/app/shared/testing/auth-service-stub.ts

Atmire-Kristof and others added some commits Nov 26, 2018

Merge branch 'master' into response-cache-refactoring
Conflicts:
	src/app/core/data/comcol-data.service.ts
	src/server.ts
Merge branch 'response-cache-refactoring' into w2p-55946_Item-mapping…
…-on-item-level

Conflicts:
	src/app/core/cache/builders/remote-data-build.service.ts
Merge branch 'master' into w2p-55946_Item-mapping-on-item-level
Conflicts:
	src/app/+community-page/community-page.component.html
	src/app/app.reducer.ts
	src/app/core/auth/server-auth.service.ts
	src/app/core/core.module.ts
	src/app/core/index/index.effects.ts
	src/app/core/index/index.reducer.spec.ts
	src/app/core/index/index.reducer.ts
	src/app/header/header.component.spec.ts
	src/app/shared/shared.module.ts
@tdonohue
Copy link
Member

tdonohue left a comment

I began to give this a code review today, and added a few inline comments below. However, I'm seeing a lot of seemingly unrelated changes in this PR which almost look like this PR's branch is not updated to use Angular 6.

@artlowel or @LotteHofstede could you take a closer look at this PR? The changes to various (seemingly unrelated) components/services seem rather odd to me, as they look to be reverting recent changes.

msToLive: 15 * 60 * 1000, // 15 minutes
msToLive: {
default: 15 * 60 * 1000, // 15 minutes
exportToZip: 5 * 1000 // 5 seconds

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 9, 2019

Member

It's unclear to me what these new settings in environment.default.js are for. Are these an unrelated change, or maybe local configuration?

This comment has been minimized.

Copy link
@artlowel

artlowel Jan 10, 2019

Member

This PR includes the same response cache refactor @LotteHofstede's #349 PR is based on. It adds the ability to cache certain responses for different lengths of time. (e.g. you may want to cache most responses for 15 minutes, but for certain admin features you may want to disable the cache, or cache it a very short time) However the exportToZip config is something that snuck in from an unrelated branch i.e. the export to zip feature that is on hold atm waiting for the scripts and processes work on the REST side. We'll remove it

]
})
/**
* Collection used to map items to a collection

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 9, 2019

Member

This comment doesn't make sense. Should this say "Component used to map items to a collection"?

This comment has been minimized.

Copy link
@artlowel

artlowel Jan 10, 2019

Member

It should. We'll change it

@@ -126,7 +126,7 @@ export class SearchFacetFilterComponent implements OnInit, OnDestroy {
this.animationState = 'ready';
this.filterValues$.next(rd);
}));
this.subs.push(newValues$.pipe(first()).subscribe((rd) => {
this.subs.push(newValues$.pipe(take(1)).subscribe((rd) => {

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 9, 2019

Member

I'm not sure I understand the reason why we are switching from first() to take(1)? If this is necessary, we should also remove first from the import statement above

This comment has been minimized.

Copy link
@artlowel

artlowel Jan 10, 2019

Member

first() and take(1) are nearly identical, the key difference is that first() will expect to get at least one value, and will throw an EmptyError if the observable completes without firing one. take(1) will simply never execute the subscribe if there are no values. @Atmire-Kristof was getting these EmptyErrors in certain cases, and that's why we replaced them.

We'll remove the import for first

@@ -37,7 +37,7 @@ export class SearchFilterComponent implements OnInit {
* Else, the filter should initially be collapsed
*/
ngOnInit() {
this.getSelectedValues().pipe(first()).subscribe((isActive) => {
this.getSelectedValues().pipe(take(1)).subscribe((isActive) => {

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 9, 2019

Member

Another switch from first to take(1). Again, if this is needed, remove first from the import

This comment has been minimized.

Copy link
@artlowel

artlowel Jan 10, 2019

Member

Will do

artlowel and others added some commits Jan 10, 2019

Merge branch 'master' into Item-Collection_Mapper
Conflicts:
	resources/i18n/en.json
	src/app/+item-page/edit-item-page/edit-item-page.component.ts
	src/app/+item-page/edit-item-page/edit-item-page.module.ts
	src/app/+item-page/edit-item-page/edit-item-page.routing.module.ts
	src/app/+item-page/edit-item-page/item-status/item-status.component.html
	src/app/+item-page/edit-item-page/item-status/item-status.component.spec.ts
	src/app/+item-page/edit-item-page/item-status/item-status.component.ts
	src/app/+item-page/item-page-routing.module.ts
	src/app/core/data/item-data.service.spec.ts
	src/app/core/data/item-data.service.ts
	src/app/core/shared/operators.spec.ts
	src/app/core/shared/operators.ts
@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Jan 18, 2019

I tried to review this PR again today, but I'm having a hard time doing a code review. It looks like it contains most of the changes in #349, but seems to have possibly diverged at some point... so attempting to diff this branch with that of #349 doesn't seem to be working.

I suspect we need to get #349 updated/merged quickly (it's nearly there) and then rebase this PR to make it easier to review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.