Significant refactor of the library and a partial support for slugs #27

hunaja · 2021-05-08T03:47:34Z

I have refactored the code base to a direction that will make the it easier to read and study, at least in my opinion. This includes mapping the user classes into "models" and "managers" folders from the little folders they used to be in. I also rewrote some existing code, spending a lot of time reorganizing the if-clauses; however, there is still some work to be done for this library to be taken seriously.

For instance, all of the errors of the library should be exported to the user. Each error related to Wilma API should trivially extend WAPIError (I wonder if this should be renamed to a more concise WilmaError?), or else error handling won't be nice. Types should also be organized more logically, and in my opinion the request wrapper needs to be heavily refactored (try linting it).

Furthermore, I have implemented a partial support for Wilma servers that use slugs. This is a breaking change, but very important as I have not been able to use the library before this. Let's take this piece of code as an example of the slug-enabled library:

import { loginToWilma } from 'openwilma.js'

/** WilmaAccount is a factory class for getting all of the users that the account has access to. */
const account: WilmaAccount = await loginToWilma({ url, username, password })

/** 
  * However, currently WilmaAccount only supports fetching the first user that the user has access to. 
  * This is bad, but enough for i.e. high schoolers.
  */
const user: WilmaUser = await account.getUser()

/** You can now perform operations on the user; for example, let's try fetching their exams list. */
const exams: Array<WilmaExam> = await user.exams.list()

This procedure is same for everyone, regardless whether their Wilma instance requires user slugs or not. I think that as the only(?) currently maintained Wilma scripting library openwilma.js should thrive to be as easy to use to be possible, hiding all of the lower-level implementation details from beginners.

Esinko

First, for this library to be taken seriously, its contributors need to act professionally.

Changes made to the login code in this PR are useless as there were never "breaking changes required". The current way of authenticating users with or without "slugs" was already a part of the library. As in you can just add your "slug" to the server url provided to the library (this requirement will be removed in the future). This implementation of user selection would not work regardless, as it does not take into account the different “form keys” between users.

This PR adds extra complexity and confusion to the library, mainly because the changes made were never required to add support for "slugs", which seems to be the point of this PR (?). While it only seems to advance your plans for the library which were already discussed on Discord and rejected.

The structural changes made in this PR are useful. Which is why I recommend making a new pull request just for them. I do not deny the need for some refactoring in the library in its current state (especially in the request wrapper), but “try linting it” won’t be good enough.

developerfromjokela

There are some great structure changes, but mainly this PR made more confusion than sense.
For roles (or slugs as you called then), they are badly incomplete. I would suggest to let us do the core part of roles, as we have more experience handling with them, assuming by your code for handling slugs. You're free to make further PRs simplifying and improving roles, etc.

developerfromjokela · 2021-05-08T07:15:20Z

lib/account/login.ts

+                "Could not fetch users for this account."
+            )
+        }
+


Role parser is missing user account type, and other details that are available in HTML.

developerfromjokela · 2021-05-08T07:18:52Z

lib/types/userInfo.ts

@@ -0,0 +1,5 @@
+export interface ListedWilmaUserInfo {


Can you clarify what is this used for? I cannot understand by its name, so renaming would be nice to something more logical.

developerfromjokela · 2021-05-08T07:51:05Z

lib/account/login.ts

+            {name: "Cookie", value: "Wilma2SID=" + sessionValue}
+        ]
+    })
+


If you would like to check if this account requires roles, please do a request to /overview, without slugs, and check the account type from form key.

Better to rely on more reliable responses than messages, it becomes even more important as it's the authentication mechanism.

developerfromjokela · 2021-05-08T07:53:56Z

lib/net/apiRequest.ts

@@ -115,6 +115,11 @@ export async function request(method: string, options: RequestOptions): Promise<
            if(options.session != null){
                if(options.body.formkey == undefined) options.body.formkey = options.session.formkey
                if(options.body.secret == undefined) options.body.secret = options.session.secret
+


If you were doing POST requests, this alone would not work.
Above you can see formkey and secret. They are different for the account itself, and all its roles.
formkey and secret needs proper handling for roles.

developerfromjokela · 2021-05-08T08:16:05Z

lib/account/account.ts

+import { ListedWilmaUserInfo } from "../types/userInfo"
+
+/** A factory class for getting the users of an account. */
+class WilmaAccount {


If roles are in array, why is first one always returned? Shouldn't there be getUsers and setUser?

developerfromjokela · 2021-05-08T08:20:48Z

lib/net/apiRequest.ts

@@ -4,7 +4,7 @@ const axios = require("axios")

 // Typings
 import {RequestHeader, RequestOptions, RequestResponse} from "../types/apiRequest"
-import Errors from "../utils/error"


Could you explain what's the reason behind this change?

developerfromjokela · 2021-05-08T08:21:44Z

lib/types/starsoft.d.ts

@@ -1,4 +1,3 @@
-// Starsoft API typings


Could you explain why did you remove this comment line? Comment lines are useful for explaining ie. usage and purpose of a given code.

ahnl

Thank you for your contribution! I appreciate your consideration for ease of use and user friendliness.

Please have a look into my comment about referencing the WilmaSession.

lib/account/managers/exams.ts

ahnl · 2021-05-08T08:27:13Z

lib/account/managers/exams.ts

+    session: WilmaSession
+
+    constructor(session: WilmaSession){
+        this.session = session


This way every manager class will be having its own WilmaSession. Could there possibly be a more elegant way, referencing a single WilmaSession for all of them?

hunaja added 8 commits May 8, 2021 02:19

chore: update lockfile version and a package

fc5c756

fix: regroup account classes to models and managers

531d232

refactor: modularize login and starsoft api

8fdb72c

fix: more readable exams manager

f619fac

fix: a typo on login code

5e11b4b

feat: partitially support user slugs

18366ee

feat: new tests with dotenv

b66f06c

chore(eslint): use unix linebreaks

cc5fbab

developerfromjokela requested review from Esinko and ahnl May 8, 2021 07:19

Esinko requested changes May 8, 2021

View reviewed changes

developerfromjokela reviewed May 8, 2021

View reviewed changes

ahnl reviewed May 8, 2021

View reviewed changes

fix: Exams grade API property name

39305a5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Significant refactor of the library and a partial support for slugs #27

Significant refactor of the library and a partial support for slugs #27

hunaja commented May 8, 2021

Esinko left a comment •

edited

Loading

developerfromjokela left a comment •

edited

Loading

developerfromjokela May 8, 2021

developerfromjokela May 8, 2021

developerfromjokela May 8, 2021

developerfromjokela May 8, 2021

developerfromjokela May 8, 2021

developerfromjokela May 8, 2021

developerfromjokela May 8, 2021

developerfromjokela May 8, 2021

ahnl left a comment

ahnl May 8, 2021

Significant refactor of the library and a partial support for slugs #27

Are you sure you want to change the base?

Significant refactor of the library and a partial support for slugs #27

Conversation

hunaja commented May 8, 2021

Esinko left a comment • edited Loading

Choose a reason for hiding this comment

developerfromjokela left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahnl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Esinko left a comment •

edited

Loading

developerfromjokela left a comment •

edited

Loading