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

Implement absences #128

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

Implement absences #128

wants to merge 2 commits into from

Conversation

netlob
Copy link
Contributor

@netlob netlob commented Mar 25, 2019

tested and fully function (as expected ;))

test:
activity ✓ should fetch activities and parts (335ms)

tested and fully function (as expected ;))
@lieuwex
Copy link
Member

lieuwex commented Mar 25, 2019

So I totally forgot this, but we already had absences in the appointments function. I don't really know the reason anymore but it basically was made so that you can only fetch absences when fetching appointments. This was the same in v1.

So the question: is it useful to fetch absences without having the appointment?
If that's the case this function would be useful, but in that case you can change the absence fetch code in appointments to utilize your new function, or remove the feature of appointments entirely.

@netlob
Copy link
Contributor Author

netlob commented Mar 25, 2019

The absence function in appointments is only for that one appointment right? This gives a full array of your absences and as far as I know the absence thingy in appointments is only for the specific appointment🤔

I'll check on this later. Atleast for my use, I would like to retrieve the absences apart from the appointments, but I don't know how other users are using that function right now... Maybe double absences function😝

Cant login without ;)
@jvdoorn
Copy link
Contributor

jvdoorn commented Jun 20, 2019

MagisterJS/src/magister.js

Lines 145 to 166 in 545b633

let absencesPromise = Promise.resolve([])
if (fetchAbsences) {
const absencesUrl = `${this._personUrl}/absenties?van=${fromUrl}&tot=${toUrl}`
absencesPromise = this._privileges.needs('Absenties', 'read')
.then(() => this.http.get(absencesUrl))
.then(res => res.json())
.then(res => res.Items.map(a => new AbsenceInfo(this, a)))
if (ignoreAbsenceErrors) {
absencesPromise = absencesPromise.catch(() => [])
}
}
return Promise.all([ appointmentsPromise, absencesPromise ])
.then(([ appointments, absences ]) => {
for (const a of appointments) {
a.absenceInfo = absences.find(i => i.appointment.id === a.id)
}
return appointments
})
.then(appointments => _.sortBy(appointments, 'start'))

The method that is used right now to fetch absences is the same as your new method so as @lieuwex suggested I would indeed change the old method to use yours. Alternatively you could also opt to not fetch the absence info when fetching appointments and make a method for it in the appointment class. A pro of doing this is that you need less time to fetch appointments because I doubt absence info is used a lot. And if someone needs absence info they can call your method.

@gruijter
Copy link

Just FYI: I use absence info in my Homey app. But if I need to rewrite my app because of this change I'll do it :)

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