RobertKrajewski / Sync-my-L2P

DOWNLOAD HERE: https://github.com/RobertKrajewski/Sync-my-L2P/releases/latest
http://www.syncmyl2p.de/
GNU Lesser General Public License v3.0
81 stars 27 forks source link

Moodle #123

Closed Bergiu closed 4 years ago

Bergiu commented 5 years ago

Hi, I'm working on the moodle api for the RWTHApp and started now on extending Sync-my-L2P to work with the nearly comming moodle api. The api isn't live yet and that's why the features won't work for you for now and the added links need to be changed later. I want to know if there is the possibility to merge these changes when the moodle api goes live?

Currently implemented and working are the core features:

There are still some bugs that I will try to fix in the next days. One of that is that moodle-course-files doesn't have the same structure like l2p-files. There are no "features" (Learning Materials, Media Library...) and the folder structure is different.

RobertKrajewski commented 5 years ago

Thank you for your contribution. I looked at your code and I really liked it. As soon as I have access to the moodle api and give it a test, I will merge your changes and publish a new version of Sync-my-L2P.

Bergiu commented 5 years ago

We've planned to publish the moodle api till next week. The changes on syncmyl2p are also nearly finished. I have a question on LoginDialog::run(). In the last commit I changed the LoginDialog class and added methods to check the availability of moodle too. But I am not sure if it is correct to execute the method LoginDialog::checkForAuthentification() in both methods. Could you please review this and send me a feedback?

RobertKrajewski commented 5 years ago

I had a quick look at it. If I am correct, calling checkForAuthentication twice could cause problems as other functions like updating the list of items could be called twice, too. Therefore, checkForAuthentication should only be called, if both the current l2p API and the new moodle API are available.

Bergiu commented 5 years ago

There was no problem last week for me, because I've forgot to call make clean before. But I have fixed it now in https://github.com/RobertKrajewski/Sync-my-L2P/commit/c84499716b902c533aa6b46c213147286e93a9ce.

Also the moodle api is now on live and I've fixed all remaining bugs.

Bergiu commented 5 years ago

There is currently a bug in the moodleapi, that doesn't allow to get the files of courses that contain the pdfannotator. So don't get confused if there are some empty courses in syncmyl2p. It will be fixed with the next release of pdfannotator.

RobertKrajewski commented 5 years ago

There are currently two problems occurring in the demolernraum for SyncMyL2P:

  1. The PDF-Annotator files are shown in the response for getfiles. Can those be removed from the response until the download is fully working? Otherwise, these files will always be shown as not synchronized files.
  2. the html in the response for getfiles have a size of 0kB. Can this be fixed?
claell commented 5 years ago

@Bergiu just a quick push. I hope you are still working on this and can help fixing those points.

Bergiu commented 5 years ago

@claell, we will deploy a fix during this week.

claell commented 5 years ago

That is great to hear. Just wanted to make sure that you were notified/aware about the recent activities in this issue.

Bergiu commented 5 years ago

We've fixed it server-side in the API. Now only files and folders are in the output. This should fix the problem with PDF-Annotator and maybe the html bug. If not please give me more information about the html bug.

RobertKrajewski commented 5 years ago

Both problems are fixes. Thanks! While testing, I noticed that the "Content-length" is not set in the response header if you download a file. This is only a minor problem, as I know the file size through the getfiles api call. However, the download file size could be slightly different due to e.g. compression.

matu3ba commented 5 years ago

@Bergiu @RobertKrajewski @claell May I ask what the status is? As of today 25.03.19, 08:55 RWTH announced that they will be changing to moodle for Summer Semester 2019.

RobertKrajewski commented 5 years ago

The current version of SmL2P already includes support for Moodle. The problem described in my last post is not critical and has to be solved at server-side.

matu3ba commented 5 years ago

For me moodle is not working. Ubuntu 18.04.2 LTS

Upon AppImage start I further have the following error twice (not sure, if that is connected to that): (SyncMyL2P-2.4.0-linux.AppImage:26786): GLib-GIO-CRITICAL **: 20:14:06.992: g_dbus_proxy_new: assertion 'G_IS_DBUS_CONNECTION (connection)' failed

INFO 20:14:20.289 Moodle-Veranstaltungen empfangen ERROR 20:14:20.289 Status der Moodle-Kursinformationen nicht ok: { "Data": null, "IsError": true, "StatusCode": 1, "StatusInfo": "Ungültige Anmeldedaten. Versuchen Sie es noch einmal!" }

Bergiu commented 5 years ago

The current version of SmL2P already includes support for Moodle. The problem described in my last post is not critical and has to be solved at server-side.

I'm trying to fix this and set the content-length header, but unfortunately I don't get why it is not send. The information is there, and the header is set, but for any reason it gets removed before sending... I can easily set other headers like "Content-Disposition", "Content-Type" or even "Content-Size", but "Content-Length" gets removed.

An easy workaround would be to send the "Content-Size" header, but this is not a standard header and only a dirty workaround.

claell commented 5 years ago

@matu3ba I am not entirely sure, whether this will help you, but I also had problems with Moodle at first, because it still used the old login which was saved. I had to login again.

This is probably because the old authentication token did not contain granted access to Moodle when it was created.

stormrider3106 commented 5 years ago

So I tried and so far it seems to be working fine but course names dont get converted from html notation so "ü" gets saved as "& # 2 5 2;" but it only affects course names.

RobertKrajewski commented 5 years ago

So I tried and so far it seems to be working fine but course names dont get converted from html notation so "ü" gets saved as "& # 2 5 2;" but it only affects course names.

I created a separate issue for that https://github.com/RobertKrajewski/Sync-my-L2P/issues/135