Moodle

Creating forum discussions may redirect to wrong forum

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 2.0, 2.5.2
  • Fix Version/s: 2.4.7, 2.5.3, 2.6, FRONTEND
  • Component/s: Forum
  • Labels:
  • Environment:
    Windows 7, tested con firefox 3.5.15, chrome 7.0.517.44 and IE 8.0.7600.16385

Description

Here's what I did,

I opened in tabs the three forums that appear on the Features Demo on http://qa.moodle.net/,
here's the tab order

1. A standard .......
2. Each person ......
3. A single dis.....

When I was adding a new discussion in forum 1, after saving it and clicking the continue button I got redirected to forum 3,

then I opened in this order

1. single
2. Standard
3. 1 discuss

Added a new topic on 2 and got redirected to 3, when back on the browsers history added a new one again and got sent again to forum 3,
without closing the tabs, I wen to forum 1 and replied a comment, when back on my browsers history on tab 2 for forum 2, clicked add new discussion, after saving I got sent to forum 1,

Issue Links

has been marked as being related by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDLSITE-1061 Wrong behavior of unread checkmark function when multiple tabs open in forums

  • Minor - Minor loss of function where workaround is possible
  • Open - The issue is open and ready for the assignee to start work on it.
is duplicated by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-42168 When trying to creat several types of forums in the same unit the links mix up

  • Critical - Crashes server, loss of data, severe memory leak
  • Closed - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Helen Foster added a comment -

Willman, thanks for your report. It sounds similar to the problem reported as MDLSITE-1061. I've added Sam as a watcher in case he wishes to comment.

Show
Helen Foster added a comment - Willman, thanks for your report. It sounds similar to the problem reported as MDLSITE-1061 . I've added Sam as a watcher in case he wishes to comment.
Hide
Permalink
Jayesh Anandani added a comment - - edited

The redirection does create the problem!While new discussions are created it gets redirected to another forum but it works good for reply messages!

Show
Jayesh Anandani added a comment - - edited The redirection does create the problem!While new discussions are created it gets redirected to another forum but it works good for reply messages!
Hide
Permalink
Eric Merrill added a comment -

New posts don't have this problem, because they unset the return URL, knowing that the correct one is computed lower.

A new one is also computed for new discussions, but it basically ignored because of the structure of the the code.

My patch mimics new posts and edits, unsetting $SESSION->fromdiscussion.

Ran all @mod_forum behat tests and unit tests, passed. I don't think I can make tests for this specific problem, as it requires multiple browser tabs.

Show
Eric Merrill added a comment - New posts don't have this problem, because they unset the return URL, knowing that the correct one is computed lower. A new one is also computed for new discussions, but it basically ignored because of the structure of the the code. My patch mimics new posts and edits, unsetting $SESSION->fromdiscussion. Ran all @mod_forum behat tests and unit tests, passed. I don't think I can make tests for this specific problem, as it requires multiple browser tabs.
Hide
Permalink
Dan Poltawski added a comment -

Hi Eric,

Thanks a lot looks good.

Can I suggest that you add a comment around this line (and in fact duplicate version of it that you've seen) because its not quite clear why we're unsetting this, so it'd be good to explain it for other developers coming along.

I'm submitting this for integration.

Show
Dan Poltawski added a comment - Hi Eric, Thanks a lot looks good. Can I suggest that you add a comment around this line (and in fact duplicate version of it that you've seen) because its not quite clear why we're unsetting this, so it'd be good to explain it for other developers coming along. I'm submitting this for integration.
Hide
Permalink
Eric Merrill added a comment -

I added a brief comment to the 3 places where the unsetting happens.

Show
Eric Merrill added a comment - I added a brief comment to the 3 places where the unsetting happens.
Hide
Permalink
Dan Poltawski added a comment -

Hi Eric,

I notice you've also changed the spacing on } else if (!empty($delete)) { // User is deleting a post, but you haven't fixed the comment. Probably i'd prefer it if you leave it alone (but if you are fixing the spacing, may as well fix the missing period too.

Show
Dan Poltawski added a comment - Hi Eric, I notice you've also changed the spacing on } else if (!empty($delete)) { // User is deleting a post , but you haven't fixed the comment. Probably i'd prefer it if you leave it alone (but if you are fixing the spacing, may as well fix the missing period too.
Hide
Permalink
Eric Merrill added a comment - - edited

Yeah, didn't see that. I just backed out the change per your preference.

The OCD in me sees something like the forums, and I want to go in a clean up all the style errors.

Show
Eric Merrill added a comment - - edited Yeah, didn't see that. I just backed out the change per your preference. The OCD in me sees something like the forums, and I want to go in a clean up all the style errors.
Hide
Permalink
Dan Poltawski added a comment -

We have done those kinda cleanups wholesale (see MDL-40847), so if you are feeling brave we would probably accept it (but it can then cause problems merging between branches, which is one reason to not go around doing it it)

Show
Dan Poltawski added a comment - We have done those kinda cleanups wholesale (see MDL-40847 ), so if you are feeling brave we would probably accept it (but it can then cause problems merging between branches, which is one reason to not go around doing it it)
Hide
Permalink
Dan Poltawski added a comment -

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Show
Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
Hide
Permalink
Marina Glancy added a comment -

Thanks Eric, this was integrated in 2.4, 2.5 and master

Show
Marina Glancy added a comment - Thanks Eric, this was integrated in 2.4, 2.5 and master
Hide
Permalink
Andrew Nicols added a comment -

Passing.
I also tested posting in both (1 then B), and making sure that the bug addresses the same concerns on the confirmation page.

Show
Andrew Nicols added a comment - Passing. I also tested posting in both (1 then B), and making sure that the bug addresses the same concerns on the confirmation page.
Hide
Permalink
Eloy Lafuente (stronk7) added a comment -

Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels.

Or, if you prefer, yes, you fixed that boring issue.

Thanks anyway! Ciao

Show
Eloy Lafuente (stronk7) added a comment - Yes, it's happening (somewhere in the French Polynesia, right now). And you did it, raising Moodle to new excellency levels. Or, if you prefer, yes, you fixed that boring issue. Thanks anyway! Ciao

People

Vote (1)
Watch (5)

Dates

  • Created:
    Updated:
    Resolved: