Fix button not updating label after subscription #4195 #4283
Conversation
tdonohue
added
bug
1 APPROVAL
There was a problem hiding this comment.
@guillermo2519 : Thanks for this PR. Overall, the feature works, but I was surprised by the change in behavior of the "Subscribe" / "Unsubscribe" option on a Community/Collection page.
Before this PR:
- If you visited a Community/Collection you were already subscribed to, you'd still see the word "Subscribe", but clicking it would allow you to view/edit your current subscription.
After this PR:
- In this PR, if you visit a Community/Collection you were already subscribed to, you now see the word "Unsubscribe", and clicking it will immediately unsubscribe you from that Community/Collection. There's no longer a way to view/edit your current subscription, unless you go to you click on your account & visit "Subscriptions" in the menu.
After seeing the new behavior, I feel like I prefer the ability to see the current subscription before unsubscribing. But, I don't know what others think about this.
It makes me think that the original ticket is flawed. I'm not sure if this button should say "Unsubscribe". Maybe it should simply say "Manage Subscription" or similar, and revert back to the previous behavior (if you click it, show the current subscription to allow for editing).
That's just my personal opinion here though, so I'd want feedback from others.
Finally, I have a few notes below. You've disabled some e2e tests from running. Those need to be re-enabled.
| it('should load if you click on "Statistics" from a Collection page', () => { | ||
| cy.visit('/collections/'.concat(Cypress.env('DSPACE_TEST_COLLECTION'))); | ||
| it.skip('should load if you click on "Statistics" from a Community page', () => { | ||
| cy.visit('/communities/'.concat(Cypress.env('DSPACE_TEST_COMMUNITY'))); |
There was a problem hiding this comment.
This test should not be skipped. Please remove the "skip". Also, it looks like you modified the test to test a Community instead of a Collection. This test is for collection statistics, not community statistics.
There was a problem hiding this comment.
Thank you @tdonohue very much for your detailed feedback! 🙏
You're absolutely right regarding the unexpected change in behavior for the "Subscribe" / "Unsubscribe" button. I now realize that the current implementation may remove the user's ability to review or manage their subscription before unsubscribing. I agree that the previous behavior was more intuitive in that regard.
I'm happy to revert the current logic and instead update the button label to "Manage Subscription", which would bring users to the existing subscription dialog for viewing or editing.
I'll also re-enable the Cypress tests that were unintentionally skipped and ensure they work as expected. Apologies for the oversight.
Thanks again for your input, i’ll push the necessary changes shortly.
github-project-automation
bot
moved this from 🙋 Needs Reviewers Assigned
to 👀 Under Review
in DSpace 9.0 Release
|
Hi @guillermo2519, |
|
Hello @tdonohue, I'm running into an issue where the I confirmed that in local, everything works fine, I've attached the screenshot as evidence. However, the same test fails during the CI run without any code changes on my side. This seems to be a CI-only issue, possibly related to timing, environment setup, or differences in how assets or routes load in GitHub Actions. Has this happened before on your end? Any insight into why Cypress tests might pass locally but fail in CI would be very helpful. Thanks in advance!
|
|
@guillermo2519 : From the logs in GitHub actions, it appears that the Collection page tests are essentially "crashing" npm. I've never seen this behavior before, but it may be the sign of some unsafe code that is hitting a memory issue or loop or similar. Have you tried testing your PR in both development mode and production mode ( I don't immediately have time to help with this PR currently. but those are my best guesses. If I can find time to test this locally, I'll let you know anything that I find. Otherwise, you are also welcome to ask for other developers to help take a look at your code/PR by asking on our #dev channel in Slack or using our dspace-devel mailing list |
|
Hi @tdonohue thanks for the suggestions! I’ve just tested the PR locally using both development mode and production SSR as you recommended: *npm run build:prod *npm run serve:ssr In both cases, everything works as expected — the Collection page loads correctly and there are no crashes or visible errors. So far, I haven't been able to reproduce the npm crash locally, which suggests it might be something specific to the CI environment — possibly related to resource limits, Node version differences (my local is Node 18.x, while GitHub Actions is using Node 20), or something triggered post-test. I’ll keep investigating to determine what could be causing this issue in the CI environment. |
There was a problem hiding this comment.
@guillermo2519 : I tried testing this today, and it seems to be having odd side effects on the Community & Collection pages when running in production mode. Here's what I'm seeing...
- First, I'm using production mode (
npm run build:prod && npm run serve:ssr) with this PR installed - I login as an Administrator
- Visit a Community. The page loads very slowly. Click on one of the "browse by" options on the Community page. The page hangs and looks like this (notice how no items are loading. This particular Community has a lot of Items on my local machine)
If I reload the page, everything will briefly work again. But, if I browse to a different Community or Collection, then the page starts hanging again on the browse by options on those pages.
I've also noticed in those scenarios, the edit "dropdown" (three dots) on the Community/Collection page will be missing the "Subscribe" or "Manage Subscriptions" option in the menu.
I'm not sure what exactly is going on, but it almost seems like the page is waiting on a response to something and not getting it.
|
Hi @tdonohue thanks for testing this and for the detailed feedback. I’ll take a closer look to see what might be causing these issues in production mode. It does sound like something is hanging or not resolving properly, possibly a pending request or a missing response. I’ll investigate the performance on the Community and Collection pages, especially related to the "browse by" functionality and the missing "Subscribe / Manage Subscriptions" options. I’ll get back to you as soon as I find out more. Thanks again for reporting this! |
|
Hi @guillermo2519, |
Hi @tdonohue, I'am @jtimal partner, I like to share this PR with you.
References
Description
Fixes an issue where the "Subscribe" button label does not update to reflect the successful subscription.
Instructions for Reviewers
List of changes in this PR:
*Added logic to update the button label dynamically after a successful subscription.
*Ensured UI reflects current subscription status immediately without requiring page reload.
How to test:
*Go to a community or collection where the subscription button is available.
*Click "Subscribe".
*Confirm that the button text updates to reflect the new subscription status (e.g., "Unsubscribe").
Reload the page and verify that the subscription state is preserved.
Include guidance for how to test or review your PR. This may include: steps to reproduce a bug, screenshots or description of a new feature, or reasons behind specific changes.
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
mainbranch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lintnpm run check-circ-deps)package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.