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

Improvements (titles and school adress), correct end times for my school and fixed #41 #39

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

jvdoorn
Copy link

@jvdoorn jvdoorn commented Aug 22, 2018

My school's Magister uses abbreviations such as "netl" for "Nederlands(e taal)" which were annoying me to be honest... so I extended your script to check if we have a replacement title for it, par example it looks up "netl" in the titles.json file and fetches the proper title which is "Nederlands", if it can't be found it just uses "netl".

The result:
screen shot 2018-08-22 at 10 21 17

The titles.json file can easily be extended to include different abbreviations/names. The structure of the file should be pretty self-explanatory.

@jvdoorn
Copy link
Author

jvdoorn commented Nov 19, 2018

Alright, what I did is add a small piece of code that allows you to add your school's address. Why is this useful? Well, my phone will now tell me when it's time to leave (Apple does this automatically if there is a location included in the event).

The way the location is formatted as following now:

Lokaal XXX
Straat 10, 1234 AA Amsterdam, Nederland

It might be that in order for it to work properly you need to update the string a bit in the config.json (par example replacing the , with \n.

@jvdoorn
Copy link
Author

jvdoorn commented Nov 21, 2018

My latest commit fixes issue #41.

@jvdoorn jvdoorn changed the title Pretty course titles. Improvements (titles and school adress), correct end times for the Zwin College and fixed #41 Nov 21, 2018
@jvdoorn jvdoorn changed the title Improvements (titles and school adress), correct end times for the Zwin College and fixed #41 Improvements (titles and school adress), correct end times for my school and fixed #41 Nov 21, 2018
Copy link
Owner

@lesander lesander left a comment

Choose a reason for hiding this comment

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

Overall some great improvements to this package, thank you Julian.

I've shared some thoughts on your changes, please review my comments.

config.json Outdated Show resolved Hide resolved
firstrun.js Outdated Show resolved Hide resolved
magister-calendar.js Outdated Show resolved Hide resolved
magister-calendar.js Outdated Show resolved Hide resolved
magister-calendar.js Outdated Show resolved Hide resolved
@jvdoorn
Copy link
Author

jvdoorn commented Nov 22, 2018

Alright, so I created a folder where custom scripts can be made. Currently they are used in two situations:

  • To fetch the proper title of an appointment. This fixes what @lesander mentioned here:

    Not all schools have a delimiter of - for all their titles (see these examples).

  • It furthermore gets rid of the two giant pieces of code handeling end times and moves them to their own school's file. This fixes what @lesander mentioned here:

    Alright. So this special handling of appointment start and end times is starting to get a little out-of-scope of the main functionality of the program and 'bloated'.

Some things that need to be done now are the following:

  • Confirm nothing got broken for the following schools:
  • dspierson
  • zwin
  • Create documentation on how to extend the scripts.

@jvdoorn
Copy link
Author

jvdoorn commented Nov 22, 2018

On a side note if anyone from Zwin College ever comes across this - please confirm that the break times for the lower classes are handled properly as well, they should be, but not sure.

… and add exceptions based on either location or title. You also have the option to disable the feature.
@jvdoorn
Copy link
Author

jvdoorn commented Nov 22, 2018

Alright, to fix your last change request I updated the way the address works. You can now select a default address and add exceptions to it either based on title or location. You also have the possibility to completely disable it.

@jvdoorn
Copy link
Author

jvdoorn commented Nov 23, 2018

I added some documentation to README.md on how to configure the address and custom scripts. This PR is ready for a review again @lesander.

…present (opendag par example). This feature will allow you to remove appointments if certain criteria are met.

In the config you can tell when we should act (I don't think you'll want to cancel the appointment if you forgot your books though), change the values by simply adding or removing a '_' (underscore disbles it). Do note that if you are not permitted to skip the class (a.k.a. you just didn't show up) the appointment won't be removed.
@jvdoorn jvdoorn mentioned this pull request Dec 14, 2018
@Gusted Gusted mentioned this pull request Dec 15, 2018
…we had to update to a newer version of magister.js for this a lot of code had to be changed. Please run the code manually at least once and check a couple times if everything is being parsed correctly.
@jvdoorn
Copy link
Author

jvdoorn commented Dec 31, 2018

A while back I started getting authentication errors. Turn out Magister changed the way you are authenticated so magister.js had to be updated. In my 4 last commits I updated magister.js and adapted MagisterCalendar to work with the changes.

When you start using these changes please make sure everything is still working as expected. A lot of variables have been renamed in magister.js and functions changed to properties. If you encounter any problems please let me know and I'll take a look at them.

@jvdoorn jvdoorn mentioned this pull request Jan 6, 2019
@jvdoorn
Copy link
Author

jvdoorn commented Jan 7, 2019

Please wait with merging until #43 has been resolved.

@jvdoorn
Copy link
Author

jvdoorn commented Jan 7, 2019

#43 Has been fixed as far as I know. This pull request is ready to be merged (again).

@ChrisvanChip
Copy link

Any update?

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.

4 participants