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

Fix item-level statistics pages #1182

Merged
merged 3 commits into from
May 24, 2021

Conversation

YanaDePauw
Copy link
Contributor

References

Fixes #1116

Description

This PR fixes the issue with navigating to the statistics page from an item where the user got redirected to the item page instead of going to the statistics page.

Instructions for Reviewers

  • Navigate to an item page
  • Click on the Statistics button in the navigation bar
  • Verify that you are navigated to the statistics page of the provided item.

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.

@artlowel artlowel added bug component: statistics e/2 Estimate in hours medium priority testathon Reported by a tester during Community Testathon labels May 12, 2021
@artlowel artlowel added this to the 7.0 milestone May 12, 2021
@lgtm-com
Copy link

lgtm-com bot commented May 12, 2021

This pull request introduces 9 alerts when merging 6631980 into a87bf00 - view on LGTM.com

new alerts:

  • 9 for Unused variable, import, function or class

@tdonohue tdonohue self-requested a review May 13, 2021 14:39
@tdonohue tdonohue added the 1 APPROVAL pull request only requires a single approval to merge label May 13, 2021
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@YanaDePauw : The code here looks good & it fixes the bug. However, I was a little surprised to see no specs/tests in this PR. Is there any way we could add a spec or two to make sure this same sort of problem doesn't happen again later? (If for some reason that's overly complex we could add a ticket for this...but I'm hoping it's simple enough to add to this PR)

@lgtm-com
Copy link

lgtm-com bot commented May 20, 2021

This pull request introduces 1 alert when merging 926dd46 into 68bd8d4 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@lgtm-com
Copy link

lgtm-com bot commented May 20, 2021

This pull request introduces 1 alert when merging 579f98d into 68bd8d4 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@tdonohue tdonohue self-requested a review May 20, 2021 14:31
Copy link
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Thanks @YanaDePauw for adding some new e2e tests here. This all looks good now!

@tdonohue tdonohue merged commit f85a5e6 into DSpace:main May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 APPROVAL pull request only requires a single approval to merge bug component: statistics e/2 Estimate in hours medium priority testathon Reported by a tester during Community Testathon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Item-level statistics pages no longer working
3 participants