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

Merge common threads #1962

Merged
merged 33 commits into from Jun 1, 2018

Conversation

Projects
None yet
4 participants
@scopeInfinity
Copy link
Member

scopeInfinity commented May 16, 2018

Issue #1950

Current Implementation

  • Assumes child thread must be newer than parent thread
  • Assign $merged_id as PK of root node of child thread, and set it's parent as root node of parent thread
  • Marks child thread as deleted.

Screenshots
screenshot from 2018-05-16 14-58-35

After Thread Y merged into Thread Z
screenshot from 2018-05-16 14-59-03

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented May 17, 2018

Merge Threads button will pop up list of possible parent threads.

Updated Screenshots

screenshot from 2018-05-17 21-46-40
screenshot from 2018-05-17 21-48-39

$message = "Can't find parent thread";
$this->course_db->rollback();
return false;
}

This comment has been minimized.

@andrewaikens87

andrewaikens87 May 18, 2018

Member

Lines 1929-1942 has duplicate code. It might be worth creating a function that returns a boolean to whether or not a non merged thread exists.

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented May 18, 2018

bad allign
The popup buttons don't seem to stay on the same line

@bmcutler bmcutler requested a review from jchulton May 21, 2018

@jchulton

This comment has been minimized.

Copy link
Contributor

jchulton commented May 21, 2018

One potential issue is that the attachments of the child thread are not carried over after the merge. Screenshots are below. Similarly, if a thread is an announcement, it will not be after being merge if it is the child thread. I don't remember this coming up during the meetings, so we may want to talk about it during today's meeting. Other than that a potential issue is that you can't have a newer thread as the parent of an older thread, but we discussed this at our meetings.
mergescreenshot1
mergescreenshot2
mergescreenshot3

@jchulton
Copy link
Contributor

jchulton left a comment

I don't see any problems with the code changes themselves (no weird indentation or spacing or anything like that), but there are some potential bugs that I outlined in my comments.

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented May 22, 2018

You are currently storing post ids in merged_id, please change the merged_id to contain the thread_id that the row merged into.

After discussing offline with Gagan we are going to keep the post_id here. My thought was to use the thread id to keep a log of the that it was merged into as it is more descriptive. But keeping track of the post id is more important for now (as it will help with un-merging). In the future we might want to add a column so we can keep track of both.

@scopeInfinity scopeInfinity changed the title Merge common threads [WIP] Merge common threads May 22, 2018

@bmcutler

This comment has been minimized.

Copy link
Contributor

bmcutler commented May 24, 2018

Do we lose the title of the thread that's merged? Sometimes the content/question is short and is mostly/fully contained in the title. Can we append the thread title at the start of the post content after merge.

bmcutler and others added some commits May 24, 2018

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented May 25, 2018

Thanks, @bmcutler @andrewaikens87 @jchulton for review.

Required changes have been made.

  • Refactored code to remove duplicate code
  • Fixed button alignment bug
  • Fixed attachments bug on merging
  • Checkbox confirming to make parent thread as announcement
    • Default selection is true if any among child or parent thread is announcement
  • For now, appending child thread title to content of its first post for not losing thread title.
  • Added E2E tests #1975

@scopeInfinity scopeInfinity changed the title [WIP] Merge common threads Merge common threads May 25, 2018

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented May 27, 2018

There seems to be a couple warnings with your tests. Take a look at https://stackoverflow.com/questions/24450395/python-with-selenium-invalid-warning

test_basic_operations_thread (e2e.test_forum.TestForumMergeThread) ... /opt/pyenv/versions/3.6.3/lib/python3.6/site-packages/selenium/webdriver/remote/webdriver.py:788: DeprecationWarning: use driver.switch_to.alert instead warnings.warn("use driver.switch_to.alert instead", DeprecationWarning) ok

test_forum_merge_thread (e2e.test_forum.TestForumMergeThread) ... /opt/pyenv/versions/3.6.3/lib/python3.6/site-packages/selenium/webdriver/remote/webdriver.py:788: DeprecationWarning: use driver.switch_to.alert instead warnings.warn("use driver.switch_to.alert instead", DeprecationWarning)

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented May 29, 2018

Thanks @andrewaikens87
Warnings fixed.

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented May 30, 2018

Just found an issue, you can recreate the screenshot error below by having two windows open. Have one merge a thread and the other stay on the child thread page and post something. It will post in the child thread a reply to a post in the parent thread.

view

Merge threads

after merge

Post on child thread

after post

If you take a look in the database notice that the child thread 9 has a post whose parent is a part of thread 5 (which is the parent). This is from a different thread id but has the same result.

db

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented May 30, 2018

@andrewaikens87 The bug related to publishing post on a deleted thread.
I tried fixing it by testing thread existence just before publishing post.

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented May 31, 2018

I think this is good. The only other thing that I think should be added is the column in the threads table to keep track of the merged thread id (currently we only keep track of the top post id of the child thread). I mentioned above for it to be added in the future but we should include it in this PR as it pertains to the code that is getting merged. @jchulton can you take another look? Thanks.

@jchulton

This comment has been minimized.

Copy link
Contributor

jchulton commented May 31, 2018

I'm still having the bug where you loose all attachments on the child branch when you merge: Screenshots below:
Child Thread:
thread merge p1
Parent Thread:
thread merge p2
Merged Thread:
threads merged

@jchulton

This comment has been minimized.

Copy link
Contributor

jchulton commented May 31, 2018

Maybe we should change this to a WIP PR and merge it. A lot of progress has been made and this PR seems to be getting a bit long, but I don't think we should call the merging of common threads done until it works with attachments (unless we want to specifically specify that it doesn't).

@andrewaikens87

This comment has been minimized.

Copy link
Member

andrewaikens87 commented May 31, 2018

@jchulton attachments seem to be working for me. I messaged you on slack to discuss more.

@scopeInfinity

This comment has been minimized.

Copy link
Member Author

scopeInfinity commented Jun 1, 2018

Pushed changes related to keeping track of merged_thread_id

bmcutler and others added some commits Jun 1, 2018

@bmcutler

This comment has been minimized.

Copy link
Contributor

bmcutler commented Jun 1, 2018

Nice work :)

@bmcutler bmcutler merged commit e9b0289 into master Jun 1, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@bmcutler bmcutler deleted the merge_common_thread branch Jun 1, 2018

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