feat: First checkin for the ical direct features

[dploeger]

This implements a feature I called "ical direct" offering ical export and import features without the need of an external provider.

Features:

• Exporting an event (with currently only basic information) is the first implementation done (i18n wasn't yet done)
• Exporting single events as well as a (optionall filtered) list of events
• Importing single events and complete calendars from ical

[dploeger]

@jdalsem can you find some time checking this out? Thanks.

[jdalsem]

i enabled the automated checks on this PR. You might want to fix them first. After that i will do a detailed review of the code.

At first glance it looks ok (with some minor remarks).

I was wondering how you see this working together with the AddThisEvent feature (which also supports ical downloads). Should users choose on or the other?

Maybe you could squash all your commits into one.. we do not need all the development commits in our history

[dploeger]

i enabled the automated checks on this PR. You might want to fix them first. After that i will do a detailed review of the code.

Sure. Will do, thanks.

I was wondering how you see this working together with the AddThisEvent feature (which also supports ical downloads). Should users choose on or the other?

I've made it selectable in the code. AddThisEvent doesn't support iCal imports and is a US-based company so many people will favor a local embedded version (at least we would 😅)

Maybe you could squash all your commits into one.. we do not need all the development commits in our history

Yes, of course. I usually do a rebase with fixups once everything is settled.

[jdalsem]

lol 4 comments... something went wrong probably

If this can provide the same feature as AddThisEvent, we might choose to drop that. It is just an 'easy' solution (from the past). No specific needs... maybe the links in the mail footer is special

[dploeger]

Shouldn't use the GitHub app on an unstable 5G connection apparently. 😂

Sure, the iCal export also works fine on mobile with direct import into the calendar app.

Although users who use the AddThisEvent app already might want to keep it.

[dploeger]

@jdalsem Could you please approve the workflow again. I've fixed some things.

[jdalsem]

we also expect the composer.lock file to be present (and updated with your changes)

[dploeger]

we also expect the composer.lock file to be present (and updated with your changes)

Hm. Actually did a composer update in the module file and added the composer.lock. 🤔

Was that wrong?

[dploeger]

Hm. Maybe I copied the wrong composer.lock. 😆 I'll take a look

[dploeger]

Ah. Found the right composer.lock. Sorry.

[jdalsem]

best option is to reset the lock and composer.json and do:

• composer install
• composer require your package

best practice is to not do composer update as it will update all (unrelated) packages... composer update should be something that we do in a separate commit / PR... not a big issue / blocker for now, but it might introduce unrelated issues

[jdalsem]

still not ok...

[dploeger]

Yeah, resetted the lock like you suggested now.

[jdalsem]

Now it installs correctly... some views/actions seem to be assuming too much... or not casting input correctly.. i will have a look at the rest of the PR

[dploeger]

Thanks. I'll take a look at the unit test result.

[dploeger]

If I read the result correctly, it just having a problem with calling the import ical action without specifying a correct input file. So the fix would be to check for a proper file and exiting, right?

[jdalsem]

It was a big review, but i think you are on the right track... do not let the amount of comments discourage you. I am merely trying to make sure it all works as expected.

Main concerns:

• forms (import/export) should not be part of a listing resource (next to all/mine)
• actions should do some more sanity checks and expect users to misbehave
• importing of attendees feels a privacy issue (as users might join events they did not subscribe for themselves)
• importing to random users/groups is possible. You might need to validate if that is possible / allowed

If we can get this PR merged, this will be added in a minor version of Event Manager as 'experimental'. If all works fine we might consider dropping AddEvent in the version for Elgg 7 and completely switch to this ical feature.

[dploeger]

I've fixed the first batches of comments. I'll take a look at the rest.

[dploeger]

Ah yes, the missing file-upload check. Thank you, PHPUnit. 😆

[dploeger]

@jdalsem Could you please approve the workflows again? I hope the test is fixed now.

I tried to run them locally, but I get this problem:

# ./vendor/bin/phpunit ./vendor/elgg/elgg/engine/tests/phpunit/plugins_integration
PHP Fatal error:  Uncaught Error: Class "Elgg\Actions\RegistrationIntegrationTestCase" not found in /var/www/localhost/htdocs/vendor/elgg/elgg/engine/tests/phpunit/plugins_integration/Elgg/Plugins/ActionsRegistrationIntegrationTest.php:7
Stack trace:
#0 /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/Util/FileLoader.php(66): include_once()
#1 /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/Util/FileLoader.php(49): PHPUnit\Util\FileLoader::load()
#2 /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/Framework/TestSuite.php(398): PHPUnit\Util\FileLoader::checkAndLoad()
#3 /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/Framework/TestSuite.php(537): PHPUnit\Framework\TestSuite->addTestFile()
#4 /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/Runner/BaseTestRunner.php(98): PHPUnit\Framework\TestSuite->addTestFiles()
#5 /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/TextUI/Command.php(121): PHPUnit\Runner\BaseTestRunner->getTest()
#6 /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/TextUI/Command.php(99): PHPUnit\TextUI\Command->run()
#7 /var/www/localhost/htdocs/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#8 /var/www/localhost/htdocs/vendor/bin/phpunit(122): include('...')
#9 {main}

Next PHPUnit\TextUI\RuntimeException: Class "Elgg\Actions\RegistrationIntegrationTestCase" not found in /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/TextUI/Command.php:101
Stack trace:
#0 /var/www/localhost/htdocs/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#1 /var/www/localhost/htdocs/vendor/bin/phpunit(122): include('...')
#2 {main}
  thrown in /var/www/localhost/htdocs/vendor/phpunit/phpunit/src/TextUI/Command.php on line 101

[dploeger]

I think, I'm done then so far. Please have a (final?) look. 🙂

[jdalsem]

i still see the resources files.... as far as i can tell these can be removed.. everything is using ajax forms or actions and there are no routes using these resource files

the region validation might be better if it does not throw (only skip) if the region does not exist

[dploeger]

i still see the resources files.... as far as i can tell these can be removed.. everything is using ajax forms or actions and there are no routes using these resource files

the region validation might be better if it does not throw (only skip) if the region does not exist

Okay. Did that. I think, it should still be registered as a warning or something if a region didn't match. Is there a method available for this?

[jdalsem]

Okay. Did that. I think, it should still be registered as a warning or something if a region didn't match. Is there a method available for this?

i should leave this as is... the region is not required (optional data) and it is no big deal if you skip it. I would not provide any feedback to the user.

There is something else you need to consider (what is your opinion?) and that is the following:
If you bulk import events, you now loop all events but if there is an issue with one of the events, you directly return an error response. This could cause issues if you import 100 events and there is an issue with number 99... the action looks to have failed because of the error_response, but you succesfully created 98 events... and maybe there also was not a problem with number 100... any ideas?

[dploeger]

Okay. Did that. I think, it should still be registered as a warning or something if a region didn't match. Is there a method available for this?

i should leave this as is... the region is not required (optional data) and it is no big deal if you skip it. I would not provide any feedback to the user.

Okay.

There is something else you need to consider (what is your opinion?) and that is the following: If you bulk import events, you now loop all events but if there is an issue with one of the events, you directly return an error response. This could cause issues if you import 100 events and there is an issue with number 99... the action looks to have failed because of the error_response, but you succesfully created 98 events... and maybe there also was not a problem with number 100... any ideas?

Very good question. I had that in the back of my mind as well. The common solution would be a transaction. Is that available in Elgg's database abstraction?

[jdalsem]

There is something else you need to consider (what is your opinion?) and that is the following: If you bulk import events, you now loop all events but if there is an issue with one of the events, you directly return an error response. This could cause issues if you import 100 events and there is an issue with number 99... the action looks to have failed because of the error_response, but you succesfully created 98 events... and maybe there also was not a problem with number 100... any ideas?

Very good question. I had that in the back of my mind as well. The common solution would be a transaction. Is that available in Elgg's database abstraction?

Transactions do not exist. I also do not believe that is the best approach. Why would we prevent importing 99 good events if 1 event is broken/invalid or missing data? I personally would go for the approach where the import would keep track of failures and report success with a message "Imported 99 events, skipped 1". Alternatively you would do a validation pass over all events in the import that would check if creation could become a problem and report: "Import impossible because event X does not meet the following requirement ...".

[dploeger]

It would be great if the skipped events would be logged and you can check for why they've been skipped. But that would break the usual toast notification, right?

[jdalsem]

It would be great if the skipped events would be logged and you can check for why they've been skipped. But that would break the usual toast notification, right?

Reason could be logged (you can mix error and success messages), but could cause a big list of error messages, but that might be something you could accept.

If there is a error use elgg_register_error_message(). If no events are imported the action should return an elgg_error_response... if one or more succesfull return a elgg_ok_response with "Imported x of y (total) events"

[dploeger]

Isn't there also a log that could be viewed in admin? I could log the concrete errors there and the toast would just give a success/skipped and errors

[jdalsem]

you could log to the php error log (by default not visible in Elgg, only visible for webserver admins)

elgg_log('Unable to import ical event for the following reason: <insert reason>', \Psr\Log\LogLevel::ERROR);

[dploeger]

Okay. I'll implement it like that.

[dploeger]

Actually, currently the fromEvent method doesn't actually throw an exception that would result in it being skipped when I review the code now. 😆

[jdalsem]

Actually, currently the fromEvent method doesn't actually throw an exception that would result in it being skipped when I review the code now.

That is not the only reason that import fails... there are also extra checks like these:
https://github.com/ColdTrick/event_manager/pull/189/changes#diff-52a41819d93a3349c85e1967987b56ef6dbb79e5b4ef53e12ac1446f997a9c84R46

all return error_responses until https://github.com/ColdTrick/event_manager/pull/189/changes#diff-52a41819d93a3349c85e1967987b56ef6dbb79e5b4ef53e12ac1446f997a9c84R74

[dploeger]

Yes, but this happens before any event is imported. I thinj the error_response is correct there.

[jdalsem]

Yes, but this happens before any event is imported. I thinj the error_response is correct there.

As far as i can tell (AFAICT) this happens for each event in the file, see: https://github.com/ColdTrick/event_manager/pull/189/changes#diff-52a41819d93a3349c85e1967987b56ef6dbb79e5b4ef53e12ac1446f997a9c84R36

[dploeger]

Ahh, damn. That's misplaced there. The actual check should only happen once. I'll fix it.

[jdalsem]

Can you then also remove the catch in the foreach? As you said that the toEvent will never throw...

[jdalsem]

and if it can never fail, there is no need to keep the counter...

[dploeger]

Yeah, I refactored that back and removed the exception now.

[jdalsem]

Is everything done? Did you 'test' as a user various cases? (bulk) import/export... enabled or not in the site settings... etc.

Besides some small issues i think this is ready to be merged. I can squash and merge this into master for you. I will mention this as a alternative/experimental feature in the next minor release of event manager and will look into extending/improving it for Elgg 7 to maybe completely replace AddEvent export features (or not).

[dploeger]

I think so. I've tested various workflows.

Thanks for the merge and the support!

[jdalsem]

Thank you for the patience to see this PR make it into this plugin! As of now you are a contributor of this plugin and future PR requests will automatically start all the actions (no need for me anymore to approve them). If you have any other wishes / bugfixes, feel free to submit another Pull Request!