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

WIP towards Elgg 1.9 version. #6

Closed

Conversation

juho-jaakkola
Copy link
Contributor

This is just to discuss current state and to provide something to test. Do not merge anything.

  • Add translations to auto-generated strings
    • admin:comment_tracker:upgrade1 etc.
  • Figure out what to do to the auto subscribe feature
    • All the stuff using COMMENT_TRACKER_UNSUBSCRIBE_RELATIONSHIP
  • Update CHANGES.txt
  • Prevent admin from running subscriptions upgrade before the settings upgrade
  • Mark upgrades done
  • Remove unnecessary functions
  • Remove debug code
    • elgg_dump
    • register_error in Upgrades_CommentTrackerUpgrade1->run()
  • Remove unneeded code in views/default/js/
  • Convert javascript into ADM module
  • Subscribing fails if usersettings include a notification method that's currently not available (the notifications plugin is disabled)

@mrclay
Copy link
Contributor

mrclay commented Apr 11, 2014

@juho-jaakkola how functional is this?

@juho-jaakkola
Copy link
Contributor Author

@mrclay I don't remember anymore. It might even be fully functional.

It however has some experimental stuff that I'm not yet sure about. See details here: #5


$notification_handlers = _elgg_services()->notifications->getMethods();

$class_name = get_input('upgrade_class');
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to whitelist available classnames.

@juho-jaakkola
Copy link
Contributor Author

@mrclay Oops, seems like this wasn't that functional after all. :P

Feel free to replace the PR if you are in a hurry. I'm traveling a bit so I can't work on this anytime soon.

@mrclay
Copy link
Contributor

mrclay commented Apr 28, 2014

Yeah, I'm just making notes for myself for when I work on this a little later.

@juho-jaakkola
Copy link
Contributor Author

I made some updates:

The 1.8 branch has received some updates while this PR has been open. It seems the branches do not have much in common anymore anyway, so do we mind if we loose the ability to merge up from 1.8 version to 1.9?

@beck24
Copy link
Member

beck24 commented May 21, 2014

I think given the changes to comments and notifications in 1.9 the inability to merge up is pretty much expected

@juho-jaakkola
Copy link
Contributor Author

I updated the PR to make use of the ElggUpgrade objects. (Which are still however broken.)

@juho-jaakkola
Copy link
Contributor Author

@beck24 @mrclay I'm currently working on this quite actively so don't fork this PR because I may be squashing commits which will break the history.

@juho-jaakkola
Copy link
Contributor Author

@mrclay @beck24

I realized I may have made a bad decision while upgrading comment_tracker.

In Elgg 1.8 comment_tracker saves a relationship between user and the target. When a comment is posted to the target, comment_tracker checks the user's notification settings and sends notifications.

In 1.9 I made it work so that it gets the user's notification settings already at the time when user subscribes. And then it creates a separate relationship between user and target for each method (notifysite, notifyemail, etc). These relationships are what Elgg 1.9 notifications system uses by default.

With this there is the problem, that the relationships do not get updated when user changes the notification methods that comment_tracker should be using.

What do you think about this? Should I revert it and use the original way? (It would still be possible to use it with the 1.9 notifications system.)

My original reasoning for doing it like this was to allow choosing content specific notification methods: #5

@mrclay
Copy link
Contributor

mrclay commented Jul 1, 2014

FWIW I stripped your PR branch to only notifyemail relationships and assumed a fresh 1.9 install. I'm still not in a position to share this yet.

@juho-jaakkola
Copy link
Contributor Author

I decided with @beck24 that it's best if I revert the change I made.

I'll open a new PR for the different approach. I shouldn't be making this PR from my own master anyway.

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.

None yet

3 participants