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

Bug in Forum #350

Closed
TimidScript opened this issue Sep 29, 2014 · 14 comments
Closed

Bug in Forum #350

TimidScript opened this issue Sep 29, 2014 · 14 comments
Labels
bug You've guessed it... this means a bug is reported. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. DB Pertains inclusively to the Database operations. needs mitigation Needs additional followup. security Usually relates to something critical.
Milestone

Comments

@TimidScript
Copy link

If you have more the one posts with the same title there's a conflict. It always goes to the earliest post.

Go here and you will see two posts with the title "TESTING". The actual post have different content but right now they both point to the same thread.

You should really have uniquely generated ids on posts and scripts. I know it's been rejected for scripts but at least there should be some for posts. ^_^

@jerone jerone added the bug You've guessed it... this means a bug is reported. label Sep 29, 2014
@Martii
Copy link
Member

Martii commented Sep 29, 2014

Related to #135

@Martii Martii added this to the #262 milestone Sep 29, 2014
@Martii Martii added the security Usually relates to something critical. label Oct 1, 2014
@sizzlemctwizzle
Copy link
Member

An incrementing id is supposed to be appended to the path (when there are duplicates) to resolve duplicate titles. It looks like that code isn't working.

@Martii
Copy link
Member

Martii commented Oct 2, 2014

Didn't know that... I added the security label because I found two users of different cases added to the user list... don't know if that was pre case insensitive fix or not... but as a precaution... both are flagged currently by me until they get singled out.

@TimidScript
Copy link
Author

Seems I jinxed myself twice. Once for flagging and now for double posting.

Someone posted a double post on my Pixiv++ script. One issue is closed the other is open and can't be closed. ^_^

@Martii Martii added the sooner Sooner would be appreciated. label Oct 19, 2014
@Martii
Copy link
Member

Martii commented Oct 19, 2014

An incrementing id is supposed to be appended to the path (when there are duplicates) to resolve duplicate titles.

@sizzlemctwizzle
I am having difficulty locating the section of code that has this duplicate check to see what is happening (or not)...

Dev is exhibiting this behavior but on Test #342 and Test #343 discussion subject names.

@Martii
Copy link
Member

Martii commented Oct 28, 2014

@sizzlemctwizzle and others,

Besides the fact that the mongoose package is using some outdated dependencies (mquery for example)... I did find this article from late 2012 which describes a "method signature" of findOne to be function findOne (conditions, fields, options, callback)...

... in which our only current line of code that tries this as a three parameter findOne is here. NEEDS MITIGATION

Based off the suggestion there it claims to try null in the 2nd parm placement and pushing it to make it into a four... the nomenclature of the "method signature" however doesn't seem congruent with the identifiers we use.

Aside from the lack of clarity on this... the following replacement line appears to work in dev:

Discussion.findOne({ path: path }, null, params, function (aErr, aDiscussion) {

Thus reenabling the "unique" identifier as depicted at:

The "bad news" is that all posts that don't have the duplicatedId set, and should be, need to be manually corrected by @sizzlemctwizzle in the databases (both dev and pro).

The "good news" is I can release a workaround with the null value in it... however the recommendation (~adjusted for our code) that is given externally is to use:

.findOne({ path: path }).sort({duplicateId: -1}).exec(callback);

The DB connection is my ultimate least familiar portion since I'm more experienced in SQL than Mongoose... but getting there.

On a final note... when composing a new post the reason why dev #323 is stripped is because we are using a GM compatible filename clean routine which strips/replaces out the # and because of the duplicate post fix, that isn't currently working as reported in this issue, and the routine strips off the numbers at the end. To resolve this there might be a consideration of encoding it... but that's after some needed feedback from some of the others @OpenUserJs/admin regarding this issue and not this "newly discovered possible" issue.

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Oct 28, 2014
See OpenUserJS#350 (comment) for extreme debuggin/testing results... awaiting feedback

This is my recommendation short term... long term is another story.
@sizzlemctwizzle
Copy link
Member

Correcting the database shouldn't be too difficult. I can handle that.

@Martii
Copy link
Member

Martii commented Oct 28, 2014

So did you want to merge #397 or wait? The longer we wait the worse it will get and perhaps stifle discussions on pro.

@Martii Martii added DB Pertains inclusively to the Database operations. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. needs discussion Blah, blah, blah, wahh, wahh, wahh, etc. labels Oct 28, 2014
@sizzlemctwizzle
Copy link
Member

Definitely merge it now. Why let more duplicates get created?

Martii added a commit that referenced this issue Oct 28, 2014
@Martii Martii removed sooner Sooner would be appreciated. needs discussion Blah, blah, blah, wahh, wahh, wahh, etc. labels Oct 28, 2014
@Martii
Copy link
Member

Martii commented Oct 28, 2014

Retested this with topic of Test #123 on dev and adds the id increment to render a url of Test_ and Test_1 and Test_2... so that is GRRRRRREAT! :)

Retested on pro at https://openuserjs.org/discuss/TESTING_1 vs @TimidScript original at https://openuserjs.org/discuss/TESTING ... again @TimidScript sizzle will need to fixer the DB to clean that up especially in your issues.

Closing as fixed from the CODE aspect.

@Martii Martii closed this as completed Oct 28, 2014
@Martii
Copy link
Member

Martii commented Oct 28, 2014

Uggh can't close in issues still (somethings still awry on the 2nd issue not being able to be closed... see https://openuserjs.org/scripts/Marti/RFC_2606%C2%A73_-_Hello,_World!/issues ) ... reopening.

@Martii Martii reopened this Oct 28, 2014
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Oct 28, 2014
* Somebody commented this out and is needed.

Applies to OpenUserJS#350
@Martii
Copy link
Member

Martii commented Oct 28, 2014

Someone posted a double post on my Pixiv++ script. One issue is closed the other is open and can't be closed. ^_^

Successful closure on two identical issues on dev at http://localhost:8080/scripts/Marti/Hello_World_6/issues/closed ... full refresh of issues and reopen of both issues and ascending order and descending order... Additional triplicate made and everything checks okay.


Will close after redeploy and retest on pro.

@Martii
Copy link
Member

Martii commented Oct 28, 2014

Deployed, retested on pro and closing. PHEW

@Martii Martii closed this as completed Oct 28, 2014
@TimidScript
Copy link
Author

Thanks :)

@Martii Martii added the needs mitigation Needs additional followup. label Mar 2, 2016
Martii added a commit to Martii/OpenUserJS.org that referenced this issue May 6, 2018
* Migrate to newer *connect-mongo*, *mongodb*, and *mongoose*.
* *express-brute* usage migrated and retested
* Disucssions, comments, searching, listing, etc retested *(SFSG)*
* Script installs retested
* WO/UL/Import retested
* Delete op retested... db intact

NOTES:
* Dev tested
* Local pro tested
* OpenUserJS#350 retested
* These are all tied together in some fashion
* *mongoose* console warning:

``` console
WARNING: The `useMongoClient` option is no longer necessary in mongoose 5.x, please remove it.
    at handleUseMongoClient (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/mongoose/lib/connection.js:506:17)
    at NativeConnection.Connection.openUri (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/mongoose/lib/connection.js:407:7)
    at Mongoose.connect (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/mongoose/lib/index.js:207:15)
    at Object.<anonymous> (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/app.js:88:10)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)
    at bootstrap_node.js:609:3
```

Ref(s):
* https:/mongodb/node-mongodb-native/blob/b8484257e16ab129b5d1614f6c9fbf933846814d/CHANGES_3.0.0.md
Martii added a commit that referenced this issue May 6, 2018
* Migrate to newer *connect-mongo*, *mongodb*, and *mongoose*.
* *express-brute* usage migrated and retested
* Discussions, comments, searching, listing, etc retested *(SFSG)*
* Script installs retested
* Script WO/UL/Import retested
* Delete op retested... db intact

NOTES:
* Dev tested
* Local pro tested
* #350 retested
* These are all tied together in some fashion
* *mongoose* console warning:

``` console
WARNING: The `useMongoClient` option is no longer necessary in mongoose 5.x, please remove it.
    at handleUseMongoClient (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/mongoose/lib/connection.js:506:17)
    at NativeConnection.Connection.openUri (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/mongoose/lib/connection.js:407:7)
    at Mongoose.connect (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/node_modules/mongoose/lib/index.js:207:15)
    at Object.<anonymous> (~/repo/git/OpenUserJS.org/martii/OpenUserJS.org/app.js:88:10)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)
    at bootstrap_node.js:609:3
```

Ref(s):
* https:/mongodb/node-mongodb-native/blob/b8484257e16ab129b5d1614f6c9fbf933846814d/CHANGES_3.0.0.md
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Aug 25, 2019
* Stumbled upon this accidentally on dev
* Denote topic length with `maxlength`... this doesn't always prevent in every browser atm but we check manually w cleaned. Post OpenUserJS#1608
* *bootstrap*ify the inputs for topic and increase their size to maxlength

NOTE:
* Signature callback really needs to be normalized to `aErr`, `aResult` so we can create some statusError at some point instead of general redirections. i.e. inform the user what they did incorrectly

Post OpenUserJS#350
Martii added a commit that referenced this issue Aug 25, 2019
* Stumbled upon this accidentally on dev
* Denote topic length with `maxlength`... this doesn't always prevent in every browser atm but we check manually w cleaned. Post #1608
* *bootstrap*ify the inputs for topic and increase their size to maxlength

NOTE:
* Signature callback really needs to be normalized to `aErr`, `aResult` so we can create some statusError at some point instead of general redirections. i.e. inform the user what they did incorrectly

Post #350

Auto-merge
@OpenUserJS OpenUserJS locked as resolved and limited conversation to collaborators Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug You've guessed it... this means a bug is reported. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. DB Pertains inclusively to the Database operations. needs mitigation Needs additional followup. security Usually relates to something critical.
Development

No branches or pull requests

4 participants