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.

Sign up for GitHub

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

Jump to bottom

HOFF-659: update nrm to use axios #591

Merged
merged 1 commit into from
Nov 12, 2024
Merged

HOFF-659: update nrm to use axios #591

merged 1 commit into from
Nov 12, 2024

Conversation

Rhodine-orleans-lindsay
Copy link
Contributor

@Rhodine-orleans-lindsay Rhodine-orleans-lindsay commented Apr 8, 2024 •
edited
Loading

What?

  • Update Modern slavery to use version of hof which replaces request module with axios - HOFF-659

Why?

  • The request module in hof is deprecated and has vulnerabilities so has been replaced by axios.

How?

  • update hof version to use axios
  • remove request module in package.json
  • remove request-promise-native module in package.json
  • update yarn.lock
  • refactored code in save-form-session, resume-form-session, delete-form-session, are-you-sure and continue-report behaviours to use hof model instead of request
  • replace request-promise-native with axios for acceptance tests

Testing?

Screenshots (optional)

Anything Else? (optional)

  • update node image versions to fix vulnerabilities and drone build
  • update casework-resolver image to version using axios

Check list

  • I have reviewed my own pull request for linting issues (e.g. adding new lines)
  • I have written tests (if relevant) -N/A
  • I have created a JIRA number for my branch
  • I have created a JIRA number for my commit
  • I have followed the chris beams method for my commit https://cbea.ms/git-commit/
    here is an example commit
  • Ensure drone builds are green especially tests
  • I will squash the commits before merging

@Rhodine-orleans-lindsay Rhodine-orleans-lindsay marked this pull request as draft April 8, 2024 10:53
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay changed the title Hoff 659 update nrm to use axios HOFF-659: update nrm to use axios Apr 8, 2024
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 8 times, most recently from a716191 to b7766a8 Compare April 9, 2024 17:36
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 2 times, most recently from ab67f25 to 12f214b Compare July 15, 2024 12:08
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 2 times, most recently from b4cb2dc to 816bde3 Compare July 22, 2024 12:13
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 4 times, most recently from 5513123 to 1eb1d45 Compare August 19, 2024 15:29
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 11 times, most recently from c1e7d9e to 0cbad1e Compare September 4, 2024 12:21
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 2 times, most recently from c70fdfe to 50f0913 Compare September 4, 2024 13:56
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 4 times, most recently from 5ef627a to a408ddf Compare September 24, 2024 10:39
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay marked this pull request as ready for review September 24, 2024 11:28
Adesh-Ramchurn
Adesh-Ramchurn reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@Adesh-Ramchurn Adesh-Ramchurn left a comment

Choose a reason for hiding this comment

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

Hi @Rhodine-orleans-lindsay PR looks good. I added some comments on there. Let me know what you think. I can then approve this PR.

apps/nrm/behaviours/are-you-sure.js Outdated Show resolved Hide resolved
apps/nrm/behaviours/resume-form-session.js
@@ -78,6 +80,9 @@ module.exports = superclass => class extends superclass {
}
req.sessionModel.set('redirect-to-reports', false);
return super.getValues(req, res, next);
});
} catch (error) {
req.log('info', `External ID: ${externalID}, Error Saving Session: ${error}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

We have been using the logger tool from HoF, just curious as to why logger is not use in this try catch block? Logger is also not used in the save-form-session.js file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we pass a req object to a function eg. getValues(req, res, next), saveValues(req, res, next), etc, we can use req.log. In hof, req.log uses the hof logger in conjunction with another logging middleware called Morgan which allows us to access some additional information in the logs such as the session id. If a function does not use the req object, then using the hof logger is enough.

apps/nrm/index.js
@@ -158,7 +158,8 @@ module.exports = {
],
locals: { showSaveAndExit: true },
fields: ['were-they-taken-somewhere-by-their-exploiter',
'were-they-taken-somewhere-by-their-exploiter-journey-details'],
'were-they-taken-somewhere-by-their-exploiter-journey-details'
],
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an extra line that has been added here. Maybe to keep it consistent with other fields the square bracket and comma could be line 161.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I have now made changes to keep the formatting consistent with other fields where the fields are on separate lines e.g. lines 288-230. For the fields above, both fields are now on separate lines from the square brackets.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Rhodine, for clarifying these changes. Code looks good to me.

@Rhodine-orleans-lindsay Rhodine-orleans-lindsay force-pushed the HOFF-659-update-nrm-to-use-axios branch 2 times, most recently from 79da3b3 to 2812f0b Compare October 2, 2024 14:12
@Rhodine-orleans-lindsay
HOFF-659: Update NRM to use version of HOF that uses axios
837fb4a 
- update hof version to use axios
- remove request module in package.json
vulnerabilities
- update yarn.lock
- update node images and dependencies to resolve vulnerabilities
- remove deprecated request-native-promise from package.json and acceptance tests and replace with axios
- refactored code in save-form-session, resume-form-session, delete-form-session, are-you-sure and continue-report behaviours to use hof model instead of request
- update icasework-resolver image
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay merged commit 4244740 into master Nov 12, 2024
4 checks passed
@Rhodine-orleans-lindsay Rhodine-orleans-lindsay deleted the HOFF-659-update-nrm-to-use-axios branch November 12, 2024 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Adesh-Ramchurn Adesh-Ramchurn Adesh-Ramchurn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Rhodine-orleans-lindsay @Adesh-Ramchurn