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

PTV-1913 force cookie resetting, including backup cookie #3657

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

briehl
Copy link
Member

@briehl briehl commented Sep 18, 2024

Description of PR purpose/changes

There's an issue where the kbase_session_backup cookie seems to be blocked or disappears on occasion. We're not entirely sure what's causing this right now - it could be some browser policy changes with regard to how the cookie is created. What it means is that direct HTTP communication to KBase services via iframes (i.e. report view and downloads) can fail by complaining that the user isn't logged in. This forces a new backup cookie to be created on page reload.

Unit and integration tests pending, but I wanted an image made to get in narrative-dev. That and production are the only environments that'll fail this way.

Jira Ticket / Issue

Related Jira ticket: https://kbase-jira.atlassian.net/browse/PTV-1913

  • Added the Jira Ticket to the title of the PR (e.g. DATAUP-69 Adds a PR template)

Testing Instructions

  • Details for how to test the PR:
  • Tests pass locally and in GitHub Actions
  • Changes available by spinning up a local narrative and navigating to X to see Y

Dev Checklist:

  • My code follows the guidelines at https://sites.google.com/lbl.gov/trussresources/home?authuser=0
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • (JavaScript) I have run Prettier and ESLint on changed code manually or with a git precommit hook
  • (Python) I have run Ruff format and check on changed Python code manually or with a git precommit hook
  • Any dependent changes have been merged and published in downstream modules

Updating Version and Release Notes (if applicable)

Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.

Project coverage is 25.91%. Comparing base (d4530b4) to head (540f8a8).
Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
kbase-extension/static/kbase/js/api/auth.js 80.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3657      +/-   ##
===========================================
+ Coverage    25.89%   25.91%   +0.02%     
===========================================
  Files          461      461              
  Lines        46652    46653       +1     
===========================================
+ Hits         12079    12090      +11     
+ Misses       34573    34563      -10     
Files with missing lines Coverage Δ
kbase-extension/static/kbase/js/narrativeLogin.js 43.85% <100.00%> (+0.49%) ⬆️
kbase-extension/static/kbase/js/api/auth.js 92.30% <80.00%> (ø)

... and 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f28a09...540f8a8. Read the comment docs.

Comment on lines 278 to 287
// const options = {
// method: callParams.method,
// headers: {
// Authorization: token,
// 'Content-Type': 'application/json',
// },

// }
// return fetch(callString, options);

Copy link
Collaborator

Choose a reason for hiding this comment

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

??

Copy link
Member Author

Choose a reason for hiding this comment

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

The only vestige of jquery is that API call wrapped in a Promise, so I started replacing it with a fetch call. Then realized there's some downstream unpacking effects to manage. Then realized that was a silly exercise and wasn't solving anything useful so just put it back and stopped.

Copy link
Collaborator

@ialarmedalien ialarmedalien left a comment

Choose a reason for hiding this comment

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

Approving in case you want to put it up on narrative-dev to test, but the comment should be removed and tests should be added when it's confirmed that this will fix things.

Copy link

sonarcloud bot commented Sep 25, 2024

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.

2 participants