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

feat: access_token is stored in global conf not local .aio #150

Conversation

jsedding
Copy link
Contributor

@jsedding jsedding commented Aug 23, 2024

Description

This change checks where an IMS context is defined, and stores the tokens in the same location (i.e. local or global).

Furthermore, on logout, only the tokens are removed, rather than the entire IMS context being rewritten, as was the case previously. The context data is the merged view of global and local configs for a context, so rewriting it risks accidentally copying data between the local IMS context definition and the global one.

Related Issue

fixes #67

Motivation and Context

We want to leverage IMS contexts in the aio-cli-plugin-aem-rde to make usage more convenient for users.

How Has This Been Tested?

This was manually tested by verifying the date was written to and deleted from the correct configuration files. The small change in StateActionContext is only a guess/best effort. I don't know in which scenario this implementation is used, so couldn't test.

Also, existing test cases were adjusted.

Screenshots (if appropriate):

Types of changes

  • Enhancement / bug fix
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

This change checks where an IMS context is defined,
and stores the tokens in the same location (i.e. local
or global).
Furthermore, on logout, _only_ the tokens are removed,
rather than the entire IMS context being rewritten,
as was the case previously. The context data is the
merged view of global and local configs for a context,
so rewriting it risks accidentally copying data between
the local IMS context definition and the global one.
Copy link
Member

@shazron shazron left a comment

Choose a reason for hiding this comment

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

Make sure the test runner passes (coverage should be 100%)

src/ctx/ConfigCliContext.js Show resolved Hide resolved
@shazron
Copy link
Member

shazron commented Sep 13, 2024

Everything else looks good except test coverage needs to be completed:

2024-09-14_00-08-40
------------------------|---------|----------|---------|---------|-------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
------------------------|---------|----------|---------|---------|-------------------
All files               |     100 |    99.29 |     100 |     100 |
 src                    |     100 |    98.87 |     100 |     100 |
  ValidationCache.js    |     100 |      100 |     100 |     100 |
  context.js            |     100 |      100 |     100 |     100 |
  errors.js             |     100 |      100 |     100 |     100 |
  ims.js                |     100 |      100 |     100 |     100 |
  index.js              |     100 |      100 |     100 |     100 |
  token-helper.js       |     100 |    96.77 |     100 |     100 | 153
 src/ctx                |     100 |      100 |     100 |     100 |
  ConfigCliContext.js   |     100 |      100 |     100 |     100 |
  Context.js            |     100 |      100 |     100 |     100 |
  StateActionContext.js |     100 |      100 |     100 |     100 |
------------------------|---------|----------|---------|---------|-------------------
Jest: "global" coverage threshold for branches (100%) not met: 99.29%

@kwin
Copy link

kwin commented Sep 24, 2024

@jsedding Any chance for increasing the code coverage in order to unblock adobe/aio-cli-plugin-aem-rde#97?

Copy link

codecov bot commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (bb41343) to head (5348a65).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #150   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines          488       490    +2     
  Branches        68        69    +1     
=========================================
+ Hits           488       490    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@RemoLiechti
Copy link
Contributor

I have added a testcase where no local flag is passed so the default to false assignment is triggered, the coverage is now back to 100%.

@shazron shazron changed the title Issue #67 - access_token is stored in global conf not local .aio feat: access_token is stored in global conf not local .aio Sep 30, 2024
@shazron
Copy link
Member

shazron commented Sep 30, 2024

This will be released as 7.1.0

@jsedding
Copy link
Contributor Author

jsedding commented Sep 30, 2024

Thanks @RemoLiechti for completing the coverage, and thanks @shazron and @amulyakashyap09 for accepting this PR!

@shazron shazron merged commit 9b25ba8 into adobe:master Oct 1, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

access_token is stored in global conf not local .aio
6 participants