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

when player deletes HabitRPG account, they are not removed from party (and guilds?) #5253

Closed
Alys opened this issue May 21, 2015 · 13 comments
Closed

Comments

@Alys
Copy link
Contributor

Alys commented May 21, 2015

When a player deletes their HabitRPG account, they remain in the members list of the party they had been in. This can cause problems with starting quests (e.g., the quest won't start automatically once all members have accepted or rejected).

The code to delete a player's account should check to see if they are in a party and if so remove them.

We should also check to see if the same problem exists for guilds, although that's not as critical since it can't cause significant problems.

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@TheHollidayInn
Copy link
Contributor

Okay, I was taking a look into this.

I can check if the user is group, but I'm not sure if I am correctly calling the groups leave function. Can anyone help my call and pass the correct parameters? I'm looking into the Express api now, but help would be welcome!

//Check if user is in groups and remove the user from them
  var q = Group.findOne({type: 'party', members: {'$in': [user._id]}})

  q.exec(function(err, group){
    if (err) return next(err);
    if(group){
      groups.leave();
    }
  });

Here is the part where I need to pass parameters.
groups.leave();

I included the groups controller like this

var groups = require("../controllers/groups")

@crookedneighbor
Copy link
Contributor

First off, you're going to want to look at the mongoose api (v 3.8.x), not express.

I think using groups.leave() here is the wrong approach. Why not just remove the member's ID from the array and then call group.save()?

Also, is the exec call needed here? Couldn't you do:

Group.findOne({ type: 'party', members: {'$in': [user._id]}}, function (err, group) {
  if (err) // handle err
  if (group) {
    // code to pop member's id from array
    group.save();
  }
})

@crookedneighbor
Copy link
Contributor

Regarding guilds, I feel like the same should be done for guilds, but I'm not sure how easy it would be to grab all the guilds that include a user. Challenges hold onto user ids too, right?

@Alys
Copy link
Contributor Author

Alys commented May 22, 2015

@crookedneighbor Probably. I'd be happy to see only parties fixed for now, and guilds sorted out later. Even party challenges could be sorted out later. The biggest problem is dead accounts in members lists.

BTW, for the existing parties that contain dead accounts, I can do stuff in the database to tidy that up, after the code fix goes live.

@TheHollidayInn
Copy link
Contributor

Aren't the group functions in the group.js controller Express?

Also, the reason I call group.leave is because the user needs to be removed from quests and challenges.

@TheHollidayInn
Copy link
Contributor

Actually, I think what I will do is abstract the functions in groups.leave to use them. I'll send some examples.

@Alys
Copy link
Contributor Author

Alys commented Jun 19, 2015

If a guild leader deletes their account without first leaving the guild, a new leader won't be automatically chosen. This should be fixed too.

@TheHollidayInn
Copy link
Contributor

Can definitely do. Does from this line down not take care of that? https://github.com/HabitRPG/habitrpg/pull/5264/files#diff-bda86c52880f0950a5eb0e2f0538b936R405

@Alys
Copy link
Contributor Author

Alys commented Jun 20, 2015

Oh, you'd already fixed it! Sorry about that! I was in a rush with many things to do yesterday and didn't even think to look to see if it was already included in your changes.

Thank you. :)

@TheHollidayInn
Copy link
Contributor

No worries :) That code was already there, though. I am writing a test to confirm and will submit it soon!

@Jolanda5
Copy link

Jolanda5 commented May 1, 2016

Is this done or is there anything else to do here?

@Alys
Copy link
Contributor Author

Alys commented May 1, 2016

@Jolanda5 I'm not certain, but Habitica's API and related code is being rewritten at the moment, and the new code is likely to either already have a fix for this issue or make it easier for a fix to be written. I'll mark this issue as "wait" and we'll look into whether a fix is still needed when the new API is released, which should be just a few weeks away.

@paglias
Copy link
Contributor

paglias commented May 1, 2016

This is fixed in v3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants