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

#1273 no js issue #1275

Merged
merged 3 commits into from
Jul 22, 2021
Merged

#1273 no js issue #1275

merged 3 commits into from
Jul 22, 2021

Conversation

atarix83
Copy link
Contributor

References

Add references/links to any related issues or PRs. These may include:

Description

This PR resolves both issues mentioned previously. I've reverted the #1244 in order to fix the #1273 and then I've used a new approach to solve the #1179.
Since the #1179 issue was due to cache problem with request to authorizations endpoint. I've added a method to set stale all the requests to the authorizations endpoint when the store is rehydrated from SSR to CSR

Include guidance for how to test or review your PR.
Test both issues are sorted out.

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

  • 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.

@atarix83 atarix83 self-assigned this Jul 20, 2021
@atarix83 atarix83 requested review from artlowel and tdonohue July 20, 2021 09:17
Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @atarix83!

I can verify that it fixes #1273, and still fixes #1179

I just have one code comment: For clarity I wouldn't append this to the existing effect but rather make a separate one.

@@ -18,10 +19,11 @@ export class ObjectCacheEffects {
*/
@Effect() fixTimestampsOnRehydrate = this.actions$
.pipe(ofType(StoreActionTypes.REHYDRATE),
map(() => new ResetObjectCacheTimestampsAction(new Date().getTime()))
map(() => new ResetObjectCacheTimestampsAction(new Date().getTime())),
tap(() => this.authorizationsService.invalidateAuthorizationsRequestCache())
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't add this to the bottom of another, unrelated effect. I'd add a separate effect that listens to the same action, with dispatch: false especially for this.

That way the effect can have a name that reflects what it does, it makes the code easier to understand, and to describe in typedocs.

Copy link
Member

Choose a reason for hiding this comment

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

That way it can also be moved to auth.effects.ts which is likely a better fit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@tdonohue tdonohue added bug component: SEO Search Engine Optimization high priority labels Jul 20, 2021
@tdonohue tdonohue added this to the 7.0 milestone Jul 20, 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.

👍 @atarix83 : Thanks for the fix here! I've tested this thoroughly and I can verify it fixes both #1273 and #1179.

So, as soon as @artlowel 's feedback is addressed, I think this can be merged.

@atarix83 atarix83 requested a review from artlowel July 21, 2021 07:46
@tdonohue
Copy link
Member

👍 Thanks @atarix83 ! I've retested today and this is still working after the refactor. It also looks (to me) that you've addressed Art's feedback.

I'll let @artlowel give this one final review tomorrow (as he's out today) and then we'll get this merged.

Copy link
Member

@artlowel artlowel left a comment

Choose a reason for hiding this comment

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

Thanks @atarix83!

@artlowel artlowel merged commit 146ec49 into DSpace:main Jul 22, 2021
4science-it pushed a commit to 4Science/dspace-angular that referenced this pull request Jan 26, 2024
…#1275)

DSC-1484 create utility functions main

Approved-by: Davide Negretti
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component: SEO Search Engine Optimization high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI does not load when JavaScript is disabled in browser Sidemenu incomplete after Shibboleth login
3 participants