Issue joining an SFx group via invite

RESOLVED INVALID

Status

Websites Graveyard
spreadfirefox.com
RESOLVED INVALID
10 years ago
4 years ago

People

(Reporter: Mary, Unassigned)

Tracking

unspecified
x86
All

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
Hi there:  I received an invite from Jay to join the community marketing group on Spread Firefox -- I wasn't logged in at the time.  It prompted me to login in order to join (the invite had a password/username field).  I did this and it directed me to My Account page vs. directing back to the invite or the actual group.

Can we get this fixed?

Thanks!

Updated

10 years ago
Assignee: nobody → paul

Comment 1

10 years ago
Looking into this ..

I have also submitted this bug on drupal.org as this bug is reproducible on a vanilla install.

http://drupal.org/node/369676

Comment 2

10 years ago
Committed a patch to OG on drupal.org for review 

http://drupal.org/node/369676#comment-1240167
(Reporter)

Comment 3

10 years ago
Thanks!

Comment 4

10 years ago
I'll get this modification implemented on D5 and committed into SVN for testing on stage..
(Reporter)

Comment 5

10 years ago
Thanks!

Comment 6

10 years ago
Created attachment 361982 [details] [diff] [review]
Improves the user experience for a group invitee who is not logged into their account

This patch has been tested against the latest release of og-5.x-8.0.
Attachment #361982 - Flags: review?(buchanae)

Updated

10 years ago
Attachment #361982 - Attachment is patch: true
Attachment #361982 - Attachment mime type: application/octet-stream → text/plain

Comment 7

10 years ago
Comment on attachment 361982 [details] [diff] [review]
Improves the user experience for a group invitee who is not logged into their account

>Index: og.module
>===================================================================
>RCS file: /cvs/drupal-contrib/contributions/modules/og/og.module,v
>retrieving revision 1.298.2.197.2.18
>diff -r1.298.2.197.2.18 og.module
>689a690
>> 	  variable_set('og_subscribe_redirect', $gid);
>1981a1983,1989
>>       case 'login':
>>       //redirect a subscribed user to the subscription page after logging into his account
>>       if ($gid = variable_get('og_subscribe_redirect', '')) {
>> 	    variable_del('og_subscribe_redirect');
>> 	    drupal_goto("og/subscribe/$gid");	
>>       }
>>       break;

Comment 8

10 years ago
It looks as though this "bug" would need to be patched against OG as its not possible to hook in and set the "og_subscribe_redirect" variable before the user is redirected to the login page.

I have contacted Moshe to see if we can get this patch committed to OG.I'll update this ticket later today

Comment 9

10 years ago
I haven't heard from Moshe, we will resolve this problem tomorrow.

Comment 10

10 years ago
I think we will need to leave this minor UI problem out of any deadlines and correct as soon as we have this fixed in OG on drupal.org .

I am going to chase Moshe again today to try and get his decision , i can't see him saying no to this request as it is a bug fix 

Best, Paul

Comment 11

10 years ago
"Great" reply from Moshe ,

"This patch is not good. it abuses the variable system to store a single user's redirect destination. Thus, busy sites will have users redirected to a different user's destination. User specific data like destination gid is best stored in the URL or in $_SESSION. Also, I would prefer to set #redirect on the login form instead of messing with hook_user('login') which could conceivably be part of a programmatic script."

I will work on implementing the #redirect approach and resubmit a patch to Moshe...

Comment 12

10 years ago
Awaiting feedback again from Moshe.

Here is the revised patch for reference ..

Index: og.module
===================================================================
RCS file: /cvs/drupal-contrib/contributions/modules/og/og.module,v
retrieving revision 1.298.2.197.2.18
diff -r1.298.2.197.2.18 og.module
691c691
<       drupal_goto('user');
---
>       drupal_goto('user/redirect/'.$gid);
1535a1536,1545
>   if ($form_id == 'user_login_block' || $form_id == 'user_login') {
>     $form['#redirect'] = og_subscribe_redirect();
>   }
> }
> 
> function og_subscribe_redirect() {
>   if ((arg(0) == 'user') && (arg(1) == 'redirect') && is_numeric(arg(2))){
>     $gid = arg(2);
>     return 'og/subscribe/' . $gid;     
>   }

Comment 13

10 years ago
Moshe replied ..

"Thats still not qite there. I don't want to do 2 redirects and add a menu callback for this. I would have to review the code flow more to tell you what I do want. Sorry, no time at the moment. I do appreciate your work on this."

It looks as though we will have to leave this until Moshe picks this up again.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WONTFIX
Don't we still want this?
OS: Mac OS X → All
Target Milestone: --- → 3.0
(Reporter)

Comment 15

10 years ago
Yes - anything we can do?
(In reply to comment #15)
> Yes - anything we can do?

We can reopen, for starters (so we can keep track of the issue) :-)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---

Updated

10 years ago
Assignee: paul → nobody
Paul: I say we just fix it and then hope that Moshe picks up the patch for the next release, or otherwise we make a note of it and don't update OG. Sfx is scheduled for a D6 update anyway, plus the patch you wrote is short and easy.

Also, is it possible to hook into the user_login() function? (user.module:898)
If so, instead of modifying OG, we could do a check if drupal_set_message has been set by OG on the /user page, and redirect accordingly. This could be a helper module again.

Comment 18

10 years ago
(In reply to comment #17)
> Paul: I say we just fix it and then hope that Moshe picks up the patch for the
> next release, or otherwise we make a note of it and don't update OG. Sfx is
> scheduled for a D6 update anyway, plus the patch you wrote is short and easy.
> 

With contributed modules if its not possible to get improvements committed straight away on Drupal.org i agree we should patch contributed module on our servers and have the changes catalogued and explained on our wiki somewhere for transparency .

> Also, is it possible to hook into the user_login() function? (user.module:898)
> If so, instead of modifying OG, we could do a check if drupal_set_message has
> been set by OG on the /user page, and redirect accordingly. This could be a
> helper module again.

I am not sure if you can do this.
I'd really like to avoid getting off course from a module maintained elsewhere.
 I think we should help Moshe out and fix this (we need to do it right), and I
don't think it's so urgent that we need to patch our version of OG without
Moshe's commitment to patch OG the same.  Just my two cents.

I think this could be easily fixed by encoding the redirect destination into
the link to the login page.  So...

1)  User is presented with message saying they have to log in to join the
group.  They are presented with a link to log in.  That link contains the group
id in the query (?joingroupid=42).

2)  When the login page is loaded, we use hook_form_alter() to check for our
group id ($_GET['joingroupid']).  We set the form's #redirect to the join URL
(og/join/42) 

3) When the user logs in, Drupal uses #redirect automatically.

http://api.drupal.org/api/file/developer/topics/forms_api_reference.html/5#redirect

Updated

10 years ago
Assignee: nobody → paul

Comment 20

10 years ago
Thanks Alex , ill take a look at this again ..

Best, Paul

Comment 21

10 years ago
Submitted an improved patch on D.O
http://drupal.org/node/369676#comment-1323366

<       drupal_goto('user');
---
>       drupal_goto('user', 'join_group='$gid);
1535a1536,1540
>   if ($form_id == 'user_login_block' || $form_id == 'user_login') {
> 	if ($_GET['join_group']) {
>       $form['#redirect'] = 'og/subscribe/' . $_GET['join_group'];
>     }
>   }

Comment 22

10 years ago
A new user also has issues while joining a group via Invite.

Once the new user registers with spreadfirefox.com,instead of the group home page...the user is taken to the home page.The new user has to search for the group they planned to join.

Consider navigating the user to the group homepage instead of the site home page.

Comment 23

10 years ago
Thanks Krupa

Improved the previous fix and corrected this problem (nicely catched :-))

diff -r1.298.2.197.2.18 og.module
690,691c690,691
<       drupal_set_message(t('In order to join this group, you must login or register a new account. After you have successfully done so, you will need to request membership again.'));
<       drupal_goto('user');
---
>       drupal_set_message(t('In order to join this group, you must login or ' .  l(t('register a new account'), "user/register", array(), 'join_group=' . $gid)  . ' register a new account. After you have successfully done so, you will need to request membership again.'));
>       drupal_goto('user', 'join_group=' . $gid);
1535a1536,1538
>   if (($form_id == 'user_login_block' || $form_id == 'user_login' || || $form_id == 'user_register') && ($_GET['join_group'] ) {
>     $form['#redirect'] = 'og/subscribe/' . $_GET['join_group'];
>   }

I'll submit an updaetd patch on D.O ....

Comment 24

10 years ago
That should be ..

diff -r1.298.2.197.2.18 og.module
690,691c690,691
<       drupal_set_message(t('In order to join this group, you must login or register a new account. After you have successfully done so, you will need to request membership again.'));
<       drupal_goto('user');
---
>       drupal_set_message(t('In order to join this group, you must login or ' .  l(t('register a new account'), "user/register", array(), 'join_group=' . $gid)  . ' After you have successfully done so, you will need to request membership again.'));
>       drupal_goto('user', 'join_group=' . $gid);
1535a1536,1538
>   if (($form_id == 'user_login_block' || $form_id == 'user_login' || $form_id == 'user_register') && ($_GET['join_group'] ) {
>     $form['#redirect'] = 'og/subscribe/' . $_GET['join_group'];
>   }

Comment 25

10 years ago
Here is my latest comment on D.O

http://drupal.org/node/369676#comment-1342220
Paul, as per the t() api:
http://api.drupal.org/api/function/t/5
You should use placeholders for inserting links. Corrected from above:
------
drupal_set_message(t('In order to join this group, you must login or @register. After you have successfully done so, you will need to request membership again.', array('@register' => l(t('register a new account'), "user/register", array(), 'join_group=' . $gid)));
------
Oh yeah, you can make the login a link too:
------
drupal_set_message(t('In order to join this group, you must @login or @register.
After you have successfully done so, you will need to request membership
again.', array('@login' => l(t('login'), "user"), '@register' => l(t('register a new account'), "user/register",
array(), 'join_group=' . $gid)));
------
(Reporter)

Comment 28

10 years ago
How's this going?
Target Milestone: 3.0 → 3.1

Comment 29

10 years ago
Nothing back from Moshe.

Comment 30

9 years ago
I think if this patch gets into OG it will be in Drupal 6.

I would recommend patching OG and then later look to resolve this issue when we migrate to D6.

Best, Paul
Assignee: paul → nobody
(Assignee)

Updated

7 years ago
Product: Websites → Websites Graveyard
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago4 years ago
Resolution: --- → INVALID
Attachment #361982 - Flags: review?(buchanae+bugs)
You need to log in before you can comment on or make changes to this bug.