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

Add MediaWiki authentication to node #19

Merged
merged 3 commits into from
Apr 19, 2021
Merged

Add MediaWiki authentication to node #19

merged 3 commits into from
Apr 19, 2021

Conversation

slifty
Copy link
Contributor

@slifty slifty commented Apr 13, 2021

This PR sets the stage for moving MediaWiki authentication logic out of shell and into the mw2pdf code base. Specifically, it introduces a new MediaWikiSession module which will allow us to separate the auth + rendering of a given page from the collation logic.

This also begins to refactor the code base a bit, moving us to node modules and starting to add some directory structure for code organization.

The new code is not integrated / run by a calling agent, but I confirmed it works by running the following in a sandbox.js file (node sandbox.js) against an instance of torque-devenv.

import { MediaWikiSession } from './classes/MediaWikiSession.js'

async function x() {
  const session = new MediaWikiSession()
  const apiUrl = 'http://127.0.0.1/LLIIA2020/api.php'
  await session.authenticate('admin', 'admin_password', apiUrl)
  await session.generatePdf('http://127.0.0.1/LLIIA2020/index.php')
}

x()

This activates the built in module mode.  It's a step in the right ES6
direction without going full-blown babel (see issue #20).

Specifically this will allow us to use `import` and `export`
@slifty slifty changed the title WIP: Add MediaWiki authentication to node Add MediaWiki authentication to node Apr 13, 2021
@slifty
Copy link
Contributor Author

slifty commented Apr 13, 2021

I decided not to add docblocks to individual methods since I believe they're somewhat auto-documented, but would be happy to add that in if desired.

@slifty
Copy link
Contributor Author

slifty commented Apr 15, 2021

An overall thought: some methods use "options" objects (e.g. `function doSomething({ firstParam, secondParam}) where others use traditional parameters. This is not inherently incorrect, but I want to do a pass to make sure there is a rhyme / reason to it.

This StackOverflow thread is a good resource when identifying rules of thumb

UPDATE: I decided to only use the options object in one method that had more than 4 parameters.

Authentication is currently done outside of the node application and
this tool expects a cookie jar to be passed.  This actually does make
sense as an option from a generic library perspective (e.g. if mw2pdf
was url2pdf).

This change is one step in adding optional parameters so that
credentials and MediaWiki api information can be provided and the tool
will authenticate in its own session.

This does not mean we will remove the ability to accept externally
defined cookies.  We may decide to remove this logic and put it in the
SimpleBook API instead, at which point we might consider doing that
generic renaming of the package.

Issue #11
It's a library that wasn't being used anywhere.

This also brings our yarn lockfile up to speed with the dependencies
introduced by the previous commit.

Issue #11
@@ -0,0 +1,74 @@
import { v4 as uuidv4 } from 'uuid'
import puppeteer from 'puppeteer'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Little thing but lets remove this whitespace (we really need a linter soon!

@@ -0,0 +1,61 @@
import fetch from 'node-fetch'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be nice to write tests for all these utility methods, but we don't have tests set up yet. Once #20 is done lets come back and fix that deficit.

@slifty
Copy link
Contributor Author

slifty commented Apr 19, 2021

Gonna merge this in!

@slifty slifty merged commit 25148de into main Apr 19, 2021
@slifty slifty deleted the 11-node-auth branch April 19, 2021 15:13
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.

1 participant