Skip to content

Conversation

@atarix83
Copy link
Contributor

References

Description

To get the best from this logging mechanism, the Angular UI should be improved by adding to all the requests these two new custom header

  • X-CORRELATION-ID with a random generated uuid or any hard to conflict string generated at the application load and keep stable for all the browsing session

  • X-REFERRER to note the page that has generated the REST request

Instructions for Reviewers

Check in the browser network console if the new headers are attached for each request to rest server

List of changes in this PR:

  • Add the creation of an uuid at app initialization and save to a session cookie
  • Create a new interceptor in order to attach the new headers to each http request

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.

@tdonohue tdonohue added 1 APPROVAL pull request only requires a single approval to merge high priority labels Jun 25, 2021
@tdonohue tdonohue added this to the 7.0 milestone Jun 25, 2021
@tdonohue tdonohue requested review from artlowel and tdonohue June 25, 2021 17:17
@tdonohue
Copy link
Member

tdonohue commented Jun 25, 2021

Assigning this to @artlowel and I to review once it's ready (since it's small though, I flagged for 1 approval, so whoever tests it first is fine with me). Currently though, it appears to be failing some tests @atarix83 . Let us know when you feel it's ready for review.

UPDATE: It looks like the test failures might be a random hiccup with e2e tests. I've restarted them to see if they succeed the second time.

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 : I gave this a try today, and (currently) it crashes the entire UI when running yarn start. It builds successfully, but when I access the UI it never successfully loads, and on the commandline I see this error:

[10:34:33 GMT-0500 (Central Daylight Time)] Listening at http://localhost:4000/
ERROR {
  statusCode: undefined,
  statusText: undefined,
  message: "Cannot read property 'length' of undefined"
}

I suspect this issue is the same reason that the e2e tests currently all fail...the UI isn't starting up properly for some reason.

@artlowel
Copy link
Member

artlowel commented Jul 1, 2021

I can reproduce @tdonohue's issue. On the client I also get a CORS error on every request. Perhaps that is the cause for the problem, and if that's the case it may simply be an issue with the REST PR: DSpace/DSpace#3303

@tdonohue tdonohue self-requested a review July 13, 2021 16:39
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.

👍 Re-tested today with the updated backend PR (DSpace/DSpace#3303) and it's working now. As mentioned in a recent meeting, the necessary change was on the backend -- the new Http Headers needed to be added to the list of "allowedHeaders" in Spring Boot.

All in all, this looks good to me.

REMINDER TO ALL: Because our e2e tests run on the latest REST API main code, they will never succeed for this PR until the corresponding REST API PR is merged (as the main code will block the new HTTP Headers sent by this PR). So, e2e test failures can be ignored in this PR as they are expected behavior.

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!

@tdonohue
Copy link
Member

After merging the REST side PR (DSpace/DSpace#3303), all PR checks now succeed! Merging this one as well since it's at +2.

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 high priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve logging Angular implementation

3 participants