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

Remove old NarrativeManager.js #1883

Merged
merged 1 commit into from
Oct 23, 2020
Merged

Remove old NarrativeManager.js #1883

merged 1 commit into from
Oct 23, 2020

Conversation

briehl
Copy link
Member

@briehl briehl commented Oct 22, 2020

Description of PR purpose/changes

The functionality in kbase-extension/static/kbase/js/api/NarrativeManager.js has long since been moved to remote NarrativeService calls. @dauglyon encountered this earlier, and I was reminded that it wasn't actually removed from this repo's codebase. Oops.

This PR fixes that by removing the module and dangling references to it. It's only imported in one module, and there it's just initialized and never used.

Testing Instructions

  • Details for how to test the PR:
  • Tests pass in travis and locally
  • 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/truss.works/kbasetruss/development
  • 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
  • Any dependent changes have been merged and published in downstream modules
  • I have run Black and Flake8 on changed Python Code manually or with git precommit (and the travis build passes)

@@ -103,7 +103,7 @@
"google_ad_conversion": "kR9OCLas4JgBEOy2pucC"
},
"comm_wait_timeout": 600000,
"config": "narrative-refactor",
"config": "dev",
Copy link
Collaborator

Choose a reason for hiding this comment

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

deliberate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Forgot about that.

Uh, semi-deliberate. It doesn't affect anything except for a dev downloading the module. Running locally against narrative-refactor doesn't seem to work for me.

@sonarcloud
Copy link

sonarcloud bot commented Oct 22, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@lgtm-com
Copy link

lgtm-com bot commented Oct 22, 2020

This pull request fixes 1 alert when merging 6113662 into 498b63c - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

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.

LGTM

@briehl
Copy link
Member Author

briehl commented Oct 22, 2020

Once Travis testing passes, I'll merge.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 16.373% when pulling 6113662 on remove-narrativemanager into 498b63c on develop.

@briehl briehl merged commit cb1d413 into develop Oct 23, 2020
@briehl briehl deleted the remove-narrativemanager branch October 23, 2020 07:02
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.

None yet

3 participants