Issue#819

[atarix83]

References

• Fixes LocaleInterceptor blocks new request #819

Description

This PR fix issue described in this issue by avoiding that a successful RemoteData is emitted two time, with undefined payload int the first one

Instructions for Reviewers

To test this PR :

• go to the mydspace page
• open the new submission
  On current main branch scrolling down the list should show an hanging loading icon. With this Pr it should work properly

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 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]

I was under the impression we agreed in the meeting last time to disable checking for eperson languages for the time being. The check doesn't really have a point at the moment, because there isn’t a way to set an eperson language from the UI yet.

But this PR keeps the check, and gets round the issue by making changes to the way remotedata states are defined, which has the potential to affect anything that uses rest data in the entire app. That means it requires that we test everything else again, and that takes it out of the 1-Approval category for me

[tdonohue]

@atarix83 : I agree with @artlowel here. It's unclear to me why the scope of this fix was expanded.

My impression from discussions in meetings was that we all agreed that #819 should be properly fixed by the work going on to resolve #739 (which is scheduled for beta5). So, we had discussed a simple, temporary fix for beta4 of leaving out the EPerson language for now (and we can add a note in #739 to "undo" that temporary fix). Is there a reason why you didn't go with that approach? Am I misunderstanding something?

[atarix83]

@tdonohue @artlowel
you're right we have decided to simply leaving out the EPerson language, but I found out a way to finnaly resolve the issue of RemoteData emitted twice and saw this resolved the problem on LocaleService. In addition now when the cache obejct is expired RemoteData is emited as failed when currently is never emitted.
I think that it should be a general improvement, I tested it and it doesn't seem to have impact, but if you prefer to go on with the workaround I'll revert it

[tdonohue]

@atarix83 : Overall, I'm not against this change...but it seems like we'd need to carefully test it. I'm also not entirely certain whether this change will be made "obsolete" once @artlowel 's work on #739 is complete.

So, I'd appreciate @artlowel 's feedback on how this PR as-is would affect #739....there's a few possible approaches I see here:

1. If we are fairly comfortable that this PR is the correct direction for the long term (and will not need reverting after Cache redesign part 1: Don't remove expired objects #739), then let's test it very thoroughly and make sure it has no side effects & then work to merge it.
2. If we feel this PR's implementation has some promise, but needs to be tested more with the work going on for Cache redesign part 1: Don't remove expired objects #739, then maybe we should have @artlowel pull this code into his ongoing work & we'll address it as part of the solution to Cache redesign part 1: Don't remove expired objects #739. In that case, we should create a separate PR to solve LocaleInterceptor blocks new request #819 with just the simple workaround for now.
3. If this seems like it's all just temporary & it'd likely need reverting as part of the solution to Cache redesign part 1: Don't remove expired objects #739, then we should simply update this PR to be the simple workaround for now.

@artlowel, do you have any feedback on the approaches above based on the ongoing work to fix #739?

[artlowel]

It is just temporary. As detailed in #739, the logic to determine the RemoteData state will move to a reducer and RemoteDataBuildService will drastically change, perhaps not even be necessary anymore. The state of an expired RemoteData object will be "stale".

[tdonohue]

Ok, given it'd be temporary, I think we should revert this back to the original idea @atarix83 & just disable the EPerson language temporarily. You can choose to either update this PR directly, or close it an create a new one.

[atarix83]

@artlowel @tdonohue
I've reverted changes and disabled the EPerson language temporarily

[artlowel]

Thanks @atarix83

[tdonohue]

👍 Looks good to me too. Thanks @atarix83