Skip to:
Content

Opened 9 years ago

Closed 8 years ago

#2002 closed defect (bug) (fixed)

When deleting activity in permalink view page should redirect back to stream

Reported by: modemlooper Owned by: cnorris23
Milestone: 1.5 Priority: major
Severity: Version:
Component: Activity Keywords: has-patch needs-testing
Cc:

Description

If you delete a view the page just stays static and blank. It would be better if the page went back to the activity stream.

Attachments (5)

2002.001.diff (2.0 KB) - added by cnorris23 9 years ago.
2002.002.diff (5.5 KB) - added by cnorris23 8 years ago.
redirect via query arg
2002.003.diff (10.5 KB) - added by cnorris23 8 years ago.
2002.004.diff (10.0 KB) - added by DJPaul 8 years ago.
2002.005.diff (10.1 KB) - added by cnorris23 8 years ago.

Download all attachments as: .zip

Change History (29)

#1 @r-a-y
9 years ago

  • Priority changed from trivial to major
  • Type changed from enhancement to defect

Changing type to defect and priority to major as this is a big usability issue.

#2 @r-a-y
9 years ago

  • Milestone set to 1.2.1

#3 @cnorris23
9 years ago

  • Keywords needs-patch added

#4 @apeatling
9 years ago

  • Milestone changed from 1.2.3 to 1.2.2

#5 @apeatling
9 years ago

  • Milestone changed from 1.2.2 to 1.2.3

#6 @cnorris23
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

This is a first pass. It gets the basic functionality down, but the redirect variable probably needs to be sanitized. Sanitizing will need to be done in global.js, and javascript is not my strong suite. I know WP has the function wp_sanitize_redirect(), but I'm not sure how to implement it in js.

@cnorris23
9 years ago

#7 @cnorris23
9 years ago

  • Component set to Activity

#8 @paulhastings0
9 years ago

  • Summary changed from When deleting activity in permalink view page should redirct back to stream to [patch] When deleting activity in permalink view page should redirct back to stream

#9 @paulhastings0
9 years ago

  • Summary changed from [patch] When deleting activity in permalink view page should redirct back to stream to [patch] When deleting activity in permalink view page should redirect back to stream

#10 @boonebgorges
9 years ago

The patch is a good start. If the redirect is going to happen with JS (something that doesn't appear to happen anywhere else in BP, fwiw), it does need to be sanitized. (JS is not my strong suit either.) Or we could just remove the ajax delete from single activity items, so that the redirect happens in PHP. That way we don't have to worry about sanitization.

There's also a little funkiness with using wp_get_referer() for the redirect address, which I discovered as I was refreshing the page over and over again, and so the bp_redirect_url was set to the single activity item :)

#11 @DJPaul
8 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 1.3 to Future Release
  • Summary changed from [patch] When deleting activity in permalink view page should redirect back to stream to When deleting activity in permalink view page should redirect back to stream

@cnorris23
8 years ago

redirect via query arg

#12 @cnorris23
8 years ago

Updated patch to redirect single activity items in PHP rather than javascript

edit: Also adds some PHPdoc

Last edited 8 years ago by cnorris23 (previous) (diff)

#13 @cnorris23
8 years ago

  • Keywords has-patch added; needs-patch removed

#14 @DJPaul
8 years ago

  • Milestone changed from Future Release to 1.3

I don't think the attached patch is the best approach, but this could do with getting fixed for 1.3 as it's not ideal right now.

#15 @cnorris23
8 years ago

Do you have any direction on how you think it should be done?

#16 @DJPaul
8 years ago

I think I must have taken a quick look at the 001 patch, sorry. 002 is good. For 1.3, the patch could now use bp_get_root_domain(), bp_is_single_activity(), and perhaps bp_activity_get_permalink() as that will take care of the root_slug changes, too :)

#17 @cnorris23
8 years ago

Alrighty. I'll whip it up :)

#18 @cnorris23
8 years ago

  • Owner set to cnorris23
  • Status changed from new to assigned

Updating patch

@cnorris23
8 years ago

#19 @DJPaul
8 years ago

Going to put this patch in shortly -- just working on a bit of tidy up of nearby code, too

@DJPaul
8 years ago

#20 @DJPaul
8 years ago

Updated patch attached; 90% the same as cnorris23's 2002.003.diff.

It works fine with no AJAX, but it doesn't redirect with AJAX. Might need to take the approach as in 2002.001.diff for that. However, it also seemed inelegant that a lot of bp_activity_action_delete_activity() is duplicated in bp_dtheme_delete_activity(), so I started trying to add an optional argument into bp_activity_action_delete_activity(), and manually set $bp->action = 'delete' in bp_dtheme_delete_activity(), but it didn't seem to work. I've left that in the patch, and ideas welcome

@cnorris23
8 years ago

#21 @cnorris23
8 years ago

As Boone suggested (comment:10) we could just avoid the ajax altogether and let the redirect happen in php only. We don't really need the ajax delete on a single activity page anyway. Paul, I've updated your updated patch ;) to include an easy fix. Not great on back-compat, but the impact should be minimal. Since, jquery is listening for clicks with the 'delete-activity', we can just set things to 'delete-activity-single' to avoid the click event. A little hackish maybe, but it doesn't cause any design/layout issues in bp-default.

#22 @boonebgorges
8 years ago

I think 2002.005.diff is pretty close. I definitely prefer removing the AJAX delete, which is silly in the case of single items.

I thought maybe it would be possible to change the JS so it wouldn't be listening for the click, but since it's done by delegation on the parent, changing the CSS class is probably the only easy way to fix it. I think the backpat considerations are probably pretty minimal. DJPaul, what do you think?

#23 @DJPaul
8 years ago

Patch seems to work fine, and it's good and tasty.

#24 @boonebgorges
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [4337]) Redirects user back to activity stream after single activity item delete. Removes AJAX from delete link on activity permalink page. Fixes #2002. Props cnorris23

Note: See TracTickets for help on using tickets.