Remove redundant legacy code and improve overall consistency #262
Comments
|
Agreed... I'm lost. I have no clue how to do certain things. I have no problems redoing any snippets I've done but it's really difficult to figure out what to use and where. Part of my skill set is reverse engineering but I'm at a loss at this time. Applies to: |
Definitely. Even I struggle sometimes, which says something about the current state of affairs. |
|
If I could make one request... when declaring function arguments... please use |
|
That sounds good. I also plan on doing some rough documentation of the design as I go through the code. I'll put it in our wiki. But don't expect something grand since I'm focusing on the code. |
|
Well documentation is a forte of mine so I can wiki assist for sure.... even if it's something as simple/similar as |
|
+1 on the global idea. |
I hate promises. They're a sloppy way to deal with asynchronous code imo. I prefer the async library. We just need to make sure that all callback functions conform to the
I've created a milestone for this blocking issue, and I know there are several other related issues can be attached to this milestone as well. You can propose others that don't already exist if you feel they fit within the goals of this issue. I'll probably create some of my own as I encounter things that need discussion. I'll begin my initial work (planning) tonight and report my progress. I seem to do my best work when the sun goes down. |
hehe me too. |
|
If anything, removal should confirm to flagging rather than the other way around. That way it's just a boolean on off to undo a removal. |
|
I decided that rather than just jumping in head first tonight, I'd do some planning. So far this is my plan of attack:
Edit: List updated on Oct. 2nd and I'm currently working on |
|
Another one; clean up the remainings of the old ui (logic and content like images). |
* Use `a` prefix on declared parameter lists... change all samples and mirror under naming * Use `www` for Wikipedia ... this allows Wikipedia to choose the language based off browser instead of us. * Miscellaneous sentence structure enhancement. * Whitespace adjustment Applies to OpenUserJS#262
|
@sizzlemctwizzle EDIT: Btw is this branch on my repo something that I can/should get rid of? It seems to be orphaned in local but I don't know for sure because it came with initial cloning of upstream here. |
|
Sure. That'd be great.
|
* At least 1 redundant/unreachable statement found Applies to OpenUserJS#262 Also some uncaught OpenUserJS#255 because no code was executed before... but not doing all of these yet.
* Change/Add some more comments in for Ambiguous * Remove some comments added as it appears they have been identified. * Unified what comments I found that I added for 264. Applies to OpenUserJS#264 as well
* Removed one comment Applies to OpenUserJS#264 and inclusive to OpenUserJS#262
* Mostly covered from USO with a few additions/changes Applies to OpenUserJS#116
|
I just merged recent PRs along with my removal of legacy stuff. These changes have been deployed so let me know if anything is broken on the site in case I missed something during testing. |
* `a`Modelname indicates "live" Object document vs `a`ModelnameData indicates that `toObject()` is applied for plain JavaScript Object and is EXPECTED... the "old style" was due to lack of Hungarian Apps notation See OpenUserJS#264. There is inconsistency noted in the individual parsers here but leaving until further testing can be done * Line length max 100 excluding TODO/NOTE/BUG comments as those should eventually go away * Added some issue pounds to what needs to be done in these files * More STYLEGUIDE.md conformance with newlines * Top header style conformance previously done in controllers * `var` hoisting as per STYLEGUIDE.md for ES5 coding **NOTES** * All of this should be a parallel change e.g. no logic changes * Some *mu2* *(mustache)* `option`s appear unused... followup Applies to OpenUserJS#819 and OpenUserJS#262... treads on OpenUserJS#262 (comment)
* Remove the patch I put in for this portion * Create function for encoding like GH's... see helper `encoding` * Using encodeURIComponent from OpenUserJS#795 * Fix bug of Fork History showing "slugged" instead of native/raw * Fix a missing model parser item for `category` virtual model * Fix some more bugs in modelParser.js * Transform a watchpoint with AuthAs ... see third alternative at http://stackoverflow.com/questions/15825333/how-to-reload-current-page-in-express-js with `aRes.redirect(req.get('referer'));` or *express*@4.x shortcut of `aRes.redirect('back');`... but if referer is missing will default to `/` Ref: http://expressjs.com/en/api.html#res.redirect ... this still appears to be a bug in *express* for quite some time so this is a workaround * Add another series of watchpoints with *express*@4.x `redirect()` doing `escape` on occasion ... special handling of `redirect`.... this particular bug doesn't appear to make much sense * Project version bump **NOTES** * Followup with DB migration to add `fork.utf` to existing forks otherwise they show up as `/` * Followup with `discussion.path` fixes. * If *express* fixes their redirect issue can simplify `encode` to be a smarter encode where it only encodes when needed... which is where it started but had to back that out in this PR * Leaving GH import alone as it still works so far * `./app.js` has one `encodeURI` in it... leaving that be for the time being until it can be retested on production... dev isn't https so no way to test here Applies to OpenUserJS#819 and OpenUserJS#262... treads on OpenUserJS#262 (comment) among many others.
* Remove legacy namespace in all routes ... part of OpenUserJS#262 and OpenUserJS#130... officially EOL as per sizzle and confirmed by me... no redirects. * Remove the patches I put in for this portion * Create function for encoding similar to GH's... see helper `encoding` * Using encodeURIComponent from OpenUserJS#795 * Fix bug of Fork History showing "slugged" instead of native/raw * Fix category reference bug... redirect was set to the Object itself and not one of its properties * Fix some more missing object identifiers in modelParser.js ... not all are validated yet and some may still be missing for future proofing * Remove and Add some more watchpoints * Flip successful "rating updated" for Groups to debug mode and related in `./libs/modelParser.js` ... these are generating too much log traffic OpenUserJS#430 **NOTES** * [ ] Followup with DB migration to add `fork.utf` to existing forks otherwise they may not show up next to the bullet * Leaving GH import alone as it still works so far ... however a watchpoint added... GH web UI always `encodeURIComponent` just about everything... will need to reaffirm existing `encodeURI` later * The term uri is inclusive to urls as previously described in OpenUserJS#819 ... however in this context url means it could be partially encoded whereas uri means it absolutely is encoded... except for the slashes... normally those are encoded but we don't want them encoded... again following GH example. A url containing `ä` may be just that however a uri has `%E4`. You will notice that I renamed some of our identifiers from `url` to `uri`... this is on purpose to indicate that is what we are expecting. * *express* `redirect()` uses [`Location`](http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.30) and [`Content-Location`](http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.14) headers. This requires all urls to be at least URI encoded as per specs... e.g. all the browsers are adding in an `escape` for the section symbol *which may be an issue with them*. So hence why the naming of `url` vs `uri`. * The base issue of readability of the code is still there... but at least this punches out some of the bugs mentioned. * Compression/optimization hasn't occurred yet * No DB migration is necessary now... this includes fixing `Re:` linkage * The smarter encoder isn't finished yet as I may be rearranging this a bit but... **This should knock out the BLOCKING label I put on**... at least from my perspective. Applies to OpenUserJS#819 and OpenUserJS#262... treads on OpenUserJS#262 (comment) among many others.
* This will probably be needed for ordered and decrementing mass removals down the line... and uncovered a few logic issues * Add some TODOs ... since this is a work in progress guestimating what should be done... final may be slightly different * Some STYLEGUIDE.md conformance * Add an error handler in `getFlaggedListForContent`... this will tell us if a flag has been found but no user in the non-long-term... Post OpenUserJS#643 fix in Moderation eyeball **NOTES** * Tested mostly on dev and some on local pro Applies to OpenUserJS#126, OpenUserJS#93 and trounces madly upon OpenUserJS#262 (comment) ... sawwy but tiz a boog.
* Show the actual model name in stderr * Elaborated more on line notes Applies to OpenUserJS#126 and trounces profoundly upon OpenUserJS#262 (comment)OpenUserJS#262 (comment)
* Calling this removal reparse for lack of better naming * Fix bug on User removal when there is no other content * Fill in the basic construct for Comment and Discussion removal... designed so the top portion can be moved around to a separate function if needed... yes it might need some compression but not until the remaining switch cases are filled in to see what can code be reused. * Removal of an owned discussion "orphans" others comment however it's not fatal and keeps their comments around * When comment editing comes around... will need to call remove Comment and check if the Discussion needs to be removed as well. * Tested orphans to ensure they didn't get reassigned if a topic is resurrected. * May need a little tweaking on User Comments list with breadcrumbs but that is UI specific. * Very literal coding here... using `null` in all *async* fields... I'm aware of `undefined` but lessens the readability and contributor understanding... negligible impact * Watching for VPS timing issue with potential multiple comments... and it has been attempted on pro already by others... added a TODO for this and will watch stderr **NOTES** * Keeping OpenUserJS#126 open until all removal issues are with minimal coverage Applies to OpenUserJS#126 ... shamelessly treads on OpenUserJS#262 (comment)
As per [sizzles comment](#262 (comment)) as being unfavorable add in `Promise`s from a previous issue into [Restrictions section](https://github.com/OpenUserJs/OpenUserJS.org/blob/master/STYLEGUIDE.md#restrictions)
* Save a click and encourage those downvoting to do something about it. Applies to OpenUserJS#262 ... treads on OpenUserJS#262 (comment)
* Move other JSON minification to `JSON.stringify` instead of *express-minify*... applies to OpenUserJS#432 and followup for OpenUserJS#899 * Remove some legacy dead code w/ route... can cause server trip * Add some graceful failures for JSON pages... applies to OpenUserJS#37 * Stop using `res.json` as it's not very configurable... do it manually with `Content-Type` header and such * Add `virtuals` to JSON output if available * Fix `find` to `findOne` in moderation removed item routine... removes outer `Array` notation and makes it look more like the model... one would hope there would be a unique id in this case * Add a missing `charset` Applies to OpenUserJS#249 and some of OpenUserJS#262
* This is very close to production version but since it is shared it's a crap shoot when creating a sandboxed *(free but limited btw)* DB. Interesting feature of mLabs. May try again to see if we can match it. First creation wasn't close enough; 2nd time it is close.
NOTES:
* Will leave old dev db up for a bit until full retesting occurs. Basic seems to be okay with new dev db.
* The FQDN name has changed for dev mLabs at some point in history. Probably offloaded onto a different server is an educated guess via DNS.
* pwd has to be at least 6 characters and include alphanumerics now... making as general as possible
* Can only import exported collections from here. Full DB import from export isn't happy which is okay as long as metadata `_since` is preserved. Probably has to do with db name conflict and mLabs.
* Metadata `_since` is preserved however it did complain about indexes on one collection:
``` console
2019-01-03T16:20:45.014-0700 Failed: openuserjs_devel.scripts: error creating indexes for openuserjs_devel.scripts: createIndex error: The field 'safe' is not valid for an index specification. Specification: { background: true, safe: null, name: "name_1", ns: "openuserjs_devel.scripts", key: { name: 1 } }
```
* 12 system indexes in new vs old which had 17. From v1 to v2. db name auto-adjusted.
* nodejitsu user index item not inclusive. Talk about a blast from the past.
* Some misc indexes that I'm not sure why they are there were not auto-imported.
* Btw we are Eastern served... so not quite sure why the traceroute tests were going West other than general internet misdirection at this time in the backbones.
* Btw if we take this info to the VPS we may lose the dev db and probably need to establish additional limited sub-accounts
Ref:
* https://docs.mlab.com/ops/#q-why-cant-i-change-the-mongodb-version-thats-running-on-my-sandbox-database
Applies to OpenUserJS#1548 OpenUserJS#262
* This is very close to production version but since it is shared it's a crap shoot when creating a sandboxed *(free but limited btw)* DB. Interesting feature of mLabs. May try again to see if we can match it. First creation wasn't close enough; 2nd time it is close.
NOTES:
* Will leave old dev db up for a bit until full retesting occurs. Basic seems to be okay with new dev db.
* The FQDN name has changed for dev mLabs at some point in history. Probably offloaded onto a different server is an educated guess via DNS.
* pwd has to be at least 6 characters and include alphanumerics now... making as general as possible
* Can only import exported collections from here. Full DB import from export isn't happy which is okay as long as metadata `_since` is preserved. Probably has to do with db name conflict and mLabs.
* Metadata `_since` is preserved however it did complain about indexes on one collection:
``` console
2019-01-03T16:20:45.014-0700 Failed: openuserjs_devel.scripts: error creating indexes for openuserjs_devel.scripts: createIndex error: The field 'safe' is not valid for an index specification. Specification: { background: true, safe: null, name: "name_1", ns: "openuserjs_devel.scripts", key: { name: 1 } }
```
* 12 system indexes in new vs old which had 17. From v1 to v2. db name auto-adjusted.
* nodejitsu user index item not inclusive. Talk about a blast from the past.
* Some misc indexes that I'm not sure why they are there were not auto-imported.
* Btw we are Eastern served... so not quite sure why the traceroute tests were going West other than general internet misdirection at this time in the backbones.
* Btw if we take this info to the VPS we may lose the dev db and probably need to establish additional limited sub-accounts
Ref:
* https://docs.mlab.com/ops/#q-why-cant-i-change-the-mongodb-version-thats-running-on-my-sandbox-database
Applies to #1548 #262
Auto-merge
* Script up to down or down to up improperly counts script votes and messes up good/bad bar. Shouldn't affect Rating in Script or Group but will recheck a few. * At the same time move to MVC *(Currently isolated to Script e.g. parallel change)*... Fixes TODO in routes.js * Some styleguide.md conformance * Simplify some views. e.g. turn some things off when they aren't able to be used. Save b/w and possibly improve understanding for most * Still would prefer returning `err` *(aErr)* with libs but that's probably a separate PR since there are still inconsistent return callback parms * Some filling in for OpenUserJS#1548 * Some additions for OpenUserJS#430. e.g. we want to know if a DB action fails currently NOTES: * Structure mirrored and altered from `flagsLib` Applies to OpenUserJS#262 and Active Maintainer override (Sorry sizzle but needed to be done so it doesn't mess up other areas down the line) * Will run through scripts and mitigate any script `votes` counts.
* Script up to down or down to up improperly counts script votes and messes up good/bad bar. Shouldn't affect Rating in Script or Group but will recheck a few. * At the same time move to MVC *(Currently isolated to Script e.g. parallel change)*... Fixes TODO in routes.js * Some styleguide.md conformance * Simplify some views. e.g. turn some things off when they aren't able to be used. Save b/w and possibly improve understanding for most * Still would prefer returning `err` *(aErr)* with libs but that's probably a separate PR since there are still inconsistent return callback parms * Some filling in for #1548 * Some additions for #430. e.g. we want to know if a DB action fails currently NOTES: * Structure mirrored and altered from `flagsLib` Applies to #262 and Active Maintainer override (Sorry sizzle but needed to be done so it doesn't mess up other areas down the line) * Will run through scripts and mitigate any script `votes` counts. Auto-merge
Post OpenUserJS#1585 and applies to OpenUserJS#262
* Transcription error from inline to MVC. My bad. * Notate in model Applies to OpenUserJS#262 and post OpenUserJS#1585
My haphazard design decisions during initial development combined with the enhancements of @Zren have left the code base full of redundancies and inconsistencies. The current state of the code is borderline incoherent. I doubt anyone other than @Zren and myself have any clue how things are structured.
The solution is to remove all redundant legacy code and to make all existing code conform to the same pattern. This includes finishing the discussion software since its lack of the ability to be voted, flagged, and removed is inconsistent. The voting and flagging routines need to be modified to match removal (since that was the last one I implemented, it's the most well formed). These routes will be converted to POST routes and accessed via Ajax on the client-side.
This issue needs to be resolved before we can move forward so added the blocking label (also I'll be changing things all over the code base and we don't want to run into merge conflicts). Please refer to our contributing documentation for specific information on the meaning of that label.
Although I've assigned myself to this issue, I'll be pushing a branch to this repo called
issue-262. If anyone wants to help out, just mirror this branch on your own repo and let me know about any changes you want me to pull in the comments here.The text was updated successfully, but these errors were encountered: