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

Browse by date and Starts-With component #364

Merged
merged 39 commits into from Mar 8, 2019

Conversation

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

Atmire-Kristof commented Feb 21, 2019

This PR closes #248 and #254

The purpose of this PR is to add a Browse-By page for date fields as well as adding a Starts-With component to all Browse-By pages.

Browse-By-Date-Page

Currently, the only date field using this component is "dc.date.issued" a.k.a. "dateissued". The route to this page can be found at [base-url]/browse/dateissued.
This is a generic component extending from BrowseByMetadataPage and can be re-used for other date fields in the future.

Starts-With

Two new components have been added: StartsWithText and StartsWithDate, both extending from an abstract component StartsWithAbstract.

  • StartsWithText renders the alphabetic starts-with as we know it from the browse-by pages in DSpace6
  • StartsWithDate renders the starts-with containing a dropdown with years and input field as we know it from the browse-by pages in DSpace6

These components come with a decorator starts-with-decorator.ts to switch components depending on their "StartsWithType", currently being either Text or Date.
This decorator is then used in browse-by.component.ts to switch to the correct StartsWith component depending on the type provided.

The StartsWith component adds a startsWith parameter to the url to limit your search on the Browse-By pages.

Atmire-Kristof added some commits Feb 11, 2019

60168: Refactor browse-by-starts-with components to starts-with compo…
…nents + further refactoring by seperating date from text

Atmire-Kristof added some commits Feb 21, 2019

Merge branch 'master' into w2p-60168_Alphabetic-browse-widget
Conflicts:
	src/app/core/browse/browse.service.spec.ts
	src/app/core/browse/browse.service.ts
	src/app/core/data/dso-response-parsing.service.ts
	src/app/shared/shared.module.ts

Atmire-Kristof added some commits Feb 22, 2019

Merge branch 'master' into w2p-60168_Alphabetic-browse-widget
Conflicts:
	src/app/core/browse/browse.service.ts
	src/app/shared/shared.module.ts
* @returns {Observable<RemoteData<PaginatedList<Item>>>}
*/
getBrowseItemsFor(filterValue: string, options: BrowseEntrySearchOptions): Observable<RemoteData<PaginatedList<Item>>> {
const request$ = this.getBrowseDefinitions().pipe(
return this.getBrowseDefinitions().pipe(

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 22, 2019

I'm getting an IllegalArgumentException on the rest side because scope isn't defined. Scope has the value "undefined". I think the problem is related with this part of the code:

        if (isNotEmpty(options.sort)) {
          args.push(`scope=${options.scope}`);
        }

The requested URL:
http://localhost/spring-rest/api/discover/browses/dateissued/items?scope=undefined&sort=default,ASC&page=NaN&size=undefined&startsWith=2017

The logged error on DSpace (java side):

2019-02-22 14:07:32,766 WARN  org.dspace.app.rest.utils.ScopeResolver @ The given scope string undefined is not a UUID
java.lang.IllegalArgumentException: Invalid UUID string: undefined

This comment has been minimized.

Copy link
@Atmire-Kristof

Atmire-Kristof Feb 26, 2019

Author Contributor

The issue should be fixed now. I simply forgot to change the "sort" property in the check to "scope". Nice catch!

@paulo-graca
Copy link

paulo-graca left a comment

Thank you @Atmire-Kristof for this contribution. Overall, to me, it's a good as is. But I've the following requests:

  • startwith parameter should be reset after, for instance, you choose a subject, otherwise you will have different announced results and returned results. I can imagine the increasing of phone calls to our support center regarding this issue.
  • the other request I have is, as in JSPUI, the support for month in date filtering
@@ -155,6 +145,9 @@ export class BrowseService {
args.push(`page=${options.pagination.currentPage - 1}`);
args.push(`size=${options.pagination.pageSize}`);
}
if (isNotEmpty(options.startsWith)) {
args.push(`startsWith=${options.startsWith}`);

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 22, 2019

I'm getting this strange behavior of having different rows vs results in facets. You can reproduce it by following this steps and use the Atmire's database - https://www.dropbox.com/s/ovqp394y3vofnwa/entities7-test-db.sql.gz?dl=1:

Access the browse by subject page:
/browse/subject
Filter the results by letter F:
/browse/subject?startsWith=F
Choose: Financial Therapy that has 8 results
/browse/subject?startsWith=F&value=financial%20therapy
You will only see 4 records but financial therapy has 8. This is because the parameter "startsWith=F" is assumed and it's used along with the filter subject. I think the parameter shouldn't be set on this request.
I excepted to see the same results as:
/browse/subject?value=financial%20therapy

This comment has been minimized.

Copy link
@Atmire-Kristof

Atmire-Kristof Feb 26, 2019

Author Contributor

The startsWith parameter is not included in the links anymore when navigating browse entries.

this.updateStartsWithTextOptions();
}

updateStartsWithTextOptions() {

This comment has been minimized.

Copy link
@paulo-graca

paulo-graca Feb 22, 2019

Please add comments to describe this function

This comment has been minimized.

Copy link
@Atmire-Kristof

Atmire-Kristof Feb 26, 2019

Author Contributor

Added comments.

This comment has been minimized.

Copy link
@paulo-graca
<select class="form-control" (change)="setStartsWithEvent($event)">
<option [value]="-1" [selected]="!startsWith">
{{'browse.startsWith.choose_year' | translate}}
</option>

This comment has been minimized.

This comment has been minimized.

Copy link
@Atmire-Kristof

Atmire-Kristof Feb 26, 2019

Author Contributor

Added a dropdown to support adding a month to the startsWith parameter.

@paulo-graca
Copy link

paulo-graca left a comment

Thank you for considering my suggestions! This PR is fine by me.

@tdonohue
Copy link
Member

tdonohue left a comment

@Atmire-Kristof : I gave this a quick review this morning. The code is looking good, and thanks for all the TypeDocs/comments in your code! I found one interface where I'd appreciate TypeDocs being added.

I haven't had a chance to test this out, but will do so later today.

@@ -0,0 +1,7 @@
import { Config } from './config.interface';

export interface BrowseByConfig extends Config {

This comment has been minimized.

Copy link
@tdonohue

tdonohue Feb 28, 2019

Member

Could we have some TypeDocs added here, please. Especially since the oneYearLimit, fiveYearLimit fields below are not self-explanatory, so it'd be good to describe the purpose of this interface

@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Feb 28, 2019

@Atmire-Kristof and @artlowel : I've tested this today. While the StartsWith & Browse By Date functionality seems to work fine overall, I've got some concerns/questions about the design approach here.

  • First, I've noticed that with the addition of the StartsWith components, all of the Browse By pages no longer return a count of total results. I'm used to seeing the "Now showing items _ of _" and that is no longer displayed.
  • Because the "Now showing" is missing, the behavior of the StartsWith components is very odd initially, until you understand what it is doing. It actually took me a moment to realize that saying "StartsWithDate = 2011" means I'll jump to seeing everything that has an Issue Date >= 2011. I think that's the correct behavior, however it's not at all clear to the user what is going on (i.e. this is likely a usability issue, and I think the "Now showing" (or similar text) would help, as it'd be clear you jumped to a new "page").
  • Browse by Author / Subject behave differently from Browse by Title / Date. The former seem to be acting as a filter (e.g. if you enter "S" in Browse By Author or Subject, you only see Authors/Subjects starting with "S"). The latter jumps you to a page in the list (so if you enter "S" in Browse by Title, you see Titles that start with S, T, U, V, etc). This may actually be an issue with the underlying REST API (or Solr index) -- I don't recall offhand.
@artlowel

This comment has been minimized.

Copy link
Member

artlowel commented Mar 1, 2019

All of the points you noted are consequences of how the rest side works I'm afraid:

  • First, I've noticed that with the addition of the StartsWith components, all of the Browse By pages no longer return a count of total results. I'm used to seeing the "Now showing items _ of _" and that is no longer displayed.
  • Because the "Now showing" is missing, the behavior of the StartsWith components is very odd initially, until you understand what it is doing. It actually took me a moment to realize that saying "StartsWithDate = 2011" means I'll jump to seeing everything that has an Issue Date >= 2011. I think that's the correct behavior, however it's not at all clear to the user what is going on (i.e. this is likely a usability issue, and I think the "Now showing" (or similar text) would help, as it'd be clear you jumped to a new "page").

When using the startsWith param, the rest api will return the exact same pagination info for any item that would be on the same page if startsWith wasn't used. So if normally the first Title starting with 'S' would be the fourth item on the 15th page, then the pagination object the rest api returns when you ask for startsWith=S will contain number: 15, but no indication of the offset that item has within the page. So we don't have, and can't calculate the "now showing" info. This is also the reason why we can't show the more extended pagination control, because all we have to go on are the previous and next hal links.

  • Browse by Author / Subject behave differently from Browse by Title / Date. The former seem to be acting as a filter (e.g. if you enter "S" in Browse By Author or Subject, you only see Authors/Subjects starting with "S"). The latter jumps you to a page in the list (so if you enter "S" in Browse by Title, you see Titles that start with S, T, U, V, etc). This may actually be an issue with the underlying REST API (or Solr index) -- I don't recall offhand.

Again that's determined by what the rest api returns. I agree that it would be nice to make this more consistent, but this seems to be exactly the way it works in dspace 6 as well:

@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Mar 1, 2019

@artlowel : thanks for the clarifications here. I agree that it sounds like the third point is current behavior (so I'll withdraw that).

However, my first point is a change in behavior from DSpace 6.x. In DSpace 6.x we are able to display a "Now showing..." option even after jumping to a specific letter. For example, here's Browse by Title starting with "D" in the XMLUI: http://demo.dspace.org/xmlui/browse?rpp=20&etal=-1&sort_by=1&type=title&starts_with=D&order=ASC

I definitely understand that this PR isn't at fault for that change in behavior. But, I wonder if we can log this as a known bug and figure out a way to fix this in the REST API (and a future Angular UI PR) -- perhaps you could brainstorm with @benbosman on this? If we feel it's nearly impossible (or very hard) to retain that "Now showing..." text, then I feel we need some other sort of contextual information (in the UI) that helps the user figure out what this component is doing.

Currently, as I noted, the behaviour was even confusing to me (a DSpace expert). For example, in testing this PR, I went to Browse by Title and clicked on letter "F". As I had no titles starting with the letter "F", the first title I saw began with "H". I initially thought it wasn't working correctly, until I realized that what was going on. However, realizing what was going on was difficult, cause there was no way to page backwards. I literally had to clear out my search and double check if I had any titles starting with "F". So, I'm worried this UI currently has some significant usability issues (again, not really a fault of this PR alone).

So, in summary, I'm OK with accepting this PR as-is (since it seems to "work") if we can develop a plan to improve it in the future. I'd like that plan to be documented (as best we can) in tickets. We could also discuss it further in next week's meeting (as a discussion topic) if necessary.

(As a sidenote, this PR now has minor merge conflicts related to the merger of the Administrative Edit PR.)

Atmire-Kristof added some commits Mar 4, 2019

Merge branch 'master' into Browse-By-Date
Conflicts:
	config/environment.default.js
	src/config/global-config.interface.ts
@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Mar 7, 2019

@Atmire-Kristof and @artlowel : After today's meeting we decided to move ahead with this PR as-is, and I'll log a bug about this behavior.

However, I just tried to retest this PR, and I'm now finding that Browse by Title & Author are no longer working. I'm seeing empty results for both that look like this (those are empty parentheses displayed for each item):
empty-results

I'm guessing that some recent code on master must now be conflicting with this PR. Both Browse by Title & Author seem to work fine on master.

@artlowel

This comment has been minimized.

Copy link
Member

artlowel commented Mar 8, 2019

@tdonohue You were right, this was an issue due to a merge in master, that didn't show up as a merge conflict in github. Thanks for testing it again before you merged 👍

@tdonohue
Copy link
Member

tdonohue left a comment

👍 Retested, and this works now. Merging.

I'll be opening several issue tickets linking back to behavioral issues mentioned above.

@tdonohue tdonohue merged commit 1103f18 into DSpace:master Mar 8, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 78.529%
Details

@wafflebot wafflebot bot removed the needs review label Mar 8, 2019

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.