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

Fix installName vs installNameSlug references #819

Open
Martii opened this issue Nov 8, 2015 · 8 comments
Open

Fix installName vs installNameSlug references #819

Martii opened this issue Nov 8, 2015 · 8 comments
Assignees
Labels
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. sooner Sooner would be appreciated.

Comments

@Martii
Copy link
Member

Martii commented Nov 8, 2015

There's a bunch of inconsistency on this... no wonder I was thrown off... this will take a bit of time.

@Martii Martii added sooner Sooner would be appreciated. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. labels Nov 8, 2015
@Martii Martii self-assigned this Nov 8, 2015
@Martii
Copy link
Member Author

Martii commented Nov 9, 2015

@Zren
Do you know what the primary reasoning behind using the identifier extension of Slug is?... I reread everything we have on it here but seems it just appeared with the nomenclature at one point mid last year.

  1. Is it an intermediate (temporary) variable?
  2. Does it symbolize anything special compared to a non-suffixed installName ?
  3. ???

@Zren
Copy link
Contributor

Zren commented Nov 9, 2015

Install name was sometimes used with .user.js tacked on at the end. I wanted to differentiate from it.

https://github.com/OpenUserJs/OpenUserJS.org/blob/e593ee2a3e7a566721c75ef877002096a83a2faa/controllers/scriptStorage.js#L28
https://github.com/OpenUserJs/OpenUserJS.org/blob/e593ee2a3e7a566721c75ef877002096a83a2faa/controllers/scriptStorage.js#L71

A slug is what Discourse used to call a normalized string in the url. Eg: /t/1234/stickied-topic From a topic with an id of 12345 and a title of Stickied Topic.

Notice how the spaces (and other non-alphanumeric characters) are converted to dashes and everything is lowercased?

My use of the word slug is probably wrong since in our case since a slug is usually used in pair with an id in the url. Since a slug can have collisions. Eg: 'A title' => 'a-title' collides with 'a-title' => 'a-title'. Also, a forum topic's title can change.

@Martii
Copy link
Member Author

Martii commented Nov 9, 2015

Install name was sometimes used with .user.js tacked on at the end.

So you wanted installName to indicate the authorname/scriptname[.user].js and installNameSlug to indicate authorname/scriptname.

I did some web research on this and WordPress also uses this nomenclature (of slugs)... They treat the url, minus the domain which is mostly true in OUJS, as the "base" and then the "slug" on the end with human readable terms... however we have the "...Slug" treated as a base too with "base/edit`, "base/source", etc... so overall I'm not sure the term slug is applicable and a bit of a stretch... however I stopped midstream of removing "... Slug" because of its use as a final intermediate. I also saw some distinction between the trailing extension too however that is inconsistent across the controllers (hence this issue)... so I got rather confused as to the reasoning.

Thank you for sharing your insights and objective on this. I'll need to ponder some more on what to do next here.

There is going to be a time soon where I probably have to split this value into it's individual components and treat them individually to work out an end-bug that I'm hunting for its root cause... this of course will make the code a little less efficient in the short term (and possibly long term) but way more readable for finding bugs e.g. I may end up either hard coding .user.js into the necessary views (and/or perhaps the routing if possible) or removing them in the applicable controllers before processing. Thanks again.

@Zren
Copy link
Contributor

Zren commented Nov 9, 2015

I'd use scriptFilename for strings with user/scriptname.user.js and scriptSlug for user/scriptname.

@Martii
Copy link
Member Author

Martii commented Nov 9, 2015

Some (ever changing possibly) notes for this issue:

  • A little less of a stretch could be to use the term URI with this Euler diagram showing the overlaps/subsets uri opt ... where a full URL, again the domain is mostly assumed in our coding, so /base1/slug1|base2/slug2 would be either be not suffixed or suffixed with URL... and a fragment of the URL would be a URI. URN's are weird and they somewhat resemble the express routing tokens (named params lists to be specific) currently found here. ... (keep in mind slug1|base2 will most likely need to be split out for handling)

@Martii
Copy link
Member Author

Martii commented Nov 9, 2015

scriptFilename ... scriptSlug

I like the idea of making more unique variable names because grep'ping on installName can pose some issues... so perhaps making it more specific can be a goal here. :)

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Nov 17, 2015
* Some whitespace adjustment for STYLEGUIDE.md
* Drop `slug` terminology except where currently needed... this still might be completely going away but smaller steps to avoid mass breakage and increase mutual understanding
* Partial fix for an encoding component issue ... there are more TODO's needed to fix all of it
* Some controller symmetry
* Using some ...`URIComponent` as indicated in OpenUserJS#795 ... needs individual handling of username and scriptname and avoids some encoding/decoding issues

**NOTES**
* This is not done yet but getting closer
* Should be a near parallel change with a partial bug fix


Applies to OpenUserJS#819 , OpenUserJS#200, and

Originally applies to OpenUserJS#262 ... treads on OpenUserJS#262 (comment) *(preauth)*
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Nov 17, 2015
* This is a mix between WordPress base/slug nomenclature and well established file name with extension concepts where as the "file name" is considered the base name since we are using multiple paths for this identifier e.g. username and scriptname which makes up installName

Applies to OpenUserJS#819
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Nov 17, 2015
…ignature

* Streamline redirects for removing to point back to the author scripts list or general users
* One whitespace adjustment on flag controller

Applies to OpenUserJS#819

Originally applies to OpenUserJS#262 ... treads on OpenUserJS#262 (comment) *(preauth)*
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Nov 25, 2015
* Still a partial bug fix... this entire slug methodology still needs to be refactored which is going to be lengthy.
* Surprised no one noticed, including me until now, that issue counts were all zero on the badge.

Followup from OpenUserJS@1f82419

Applies to OpenUserJS#819, OpenUserJS#200, and

Originally applies to OpenUserJS#262 ... not part of OpenUserJS#262 (comment)
@Martii Martii added the DB Pertains inclusively to the Database operations. label Nov 25, 2015
@Martii
Copy link
Member Author

Martii commented Nov 25, 2015

Doing some quoting here from MDN and confirmed:

Note that encodeURI by itself cannot form proper HTTP GET and POST requests, such as for XMLHTTPRequests, because "&", "+", and "=" are not encoded, which are treated as special characters in GET and POST requests. encodeURIComponent, however, does encode these characters.

node and express are decoding GET and POST requests which wreaks havoc if certain characters are used in a discussion/script/user.

This is the failure we have between "slug"ging and URI's just in general. e.g. we must use *URIComponent to resolve these (individually handled) no matter how ick the address bar looks... still running a test but probably won't get back to this until after the holiday.

Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Dec 23, 2015
* **EXPERIMENTAL**  ES5 minification via url ... Uses same routes but `.min` prefix on extension. If ES6 is encountered it should just not minify.

**NOTES**
* ES6 minification isn't supported/stable yet but will probably request an experimental branch on *express-minify* or something else
* This is on a trial basis to see how much interest and possible clobbering happens... *uglifyjs2* claims stable support with ES5 ... this will test that. Authors should recommend debugging against original source but with a notation that minified may or may not work.
* OpenUserJS#819 followup with renaming external urls
* Debug mode of course won't use *express-minify* currently so nothing should happen on these routes.. this is being reevaluated.

* BUG fix for libraries sharing same S3 storage space when ending in a reserved extension from script name
* BUG fix for the regular expression allowing some urls without 404

Applies to OpenUserJS#432
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Feb 5, 2016
* Fix a regular expression to use environment port if available ... this is a dev bug
* Some prior typos and comments clarified
* New OpenUserJS metadata block key of `unstableMinify`... following Scriptish example of camel casing *(interCaps)*. This requires a string after it which is the reason why... invalid reasons may reduce ones karma :)
* Using boolean switch so that the view contains the strings and not the controller/libs side of our code... useful for OpenUserJS#18 and how views should eventually be rendered
* Libraries aren't covered yet because of OpenUserJS#723
* `decodeURIComponent` is required as per OpenUserJS#819 ... do a trap for now just in case it fails... which may produce a false positive but these are just notices at this time

Applies to OpenUserJS#432
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Feb 5, 2016
* OpenUserJS#819 is rearing it's ugly head here

**NOTES**
Will have to spend some more time on this for that issue but as I stated it's roughed in

Applies to OpenUserJS#432
@Martii
Copy link
Member Author

Martii commented Feb 5, 2016

Since I'm hitting this issue wall with other issues, as I expected which is why this issue is open, this has to be done first before any more new features can be added.

Adding BLOCKING to this...

This will entail a DB migration and installName will become an object in the model of strings as such:

EDITED

installName = {
  utf : 'authorname/scriptname' + '.user.js', // For DB access
  url : encodeURIComponent('authorname') + '/' +
          encodeURIComponent('scriptname') + '.user.js' // For browser access
}

... unless anyone has any other idea... I've ran most of this through @sizzlemctwizzle already and he can't think of any additional options at last chat.

Whether the naming is "slug" or not makes zero difference here other than how it is perceived... but either the utf property can be slugged e.g. GM cleaned or the url property can be slugged e.g. GM cleaned.

@Martii Martii added the BLOCKING Concentrate on bug fixes primarily. label Feb 5, 2016
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Feb 5, 2016
* Still keeping author notice since that isn't affected by OpenUserJS#819 ... when this other issue is fixed should be able to readd this back in a working function

**NOTES**
Have some log data now on just how far this reaches which is a lot

Applies to OpenUserJS#432
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Feb 17, 2016
* `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)
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Feb 20, 2016
* 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.
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Feb 21, 2016
* 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.
Martii referenced this issue in ardiman/userscripts Mar 5, 2016
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Mar 7, 2016
* Trap if a Userscript doesn't encode one of these urls correctly. Fixes "Page Load Error" with "Failed to Connect"... server trip

Applies to OpenUserJS#819, OpenUserJS#239, OpenUserJS#197, and OpenUserJS#189

Ref:
* https://openuserjs.org/discuss/webhook_not_working
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Apr 22, 2016
* Improve `@updateURL` and `downloadURL` checks

Applies to OpenUserJS#944, OpenUserJS#819 and OpenUserJS#432
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue May 6, 2016
* Additional validation on `@name`
* Fix trimming in lib model meta `name`... **NOTE** `trim()` is technically ES5.1 and is used elsewhere already... so normalizing on that

Post fix for OpenUserJS#944 and applies to OpenUserJS#432 and OpenUserJS#819... also from initial library creation
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Apr 20, 2017
* Needs lastModified for this... plus bug fix on re for OpenUserJS#819

Applies to OpenUserJS#432
Martii added a commit that referenced this issue Apr 20, 2017
* Needs lastModified for this... plus bug fix on re for #819

Applies to #432

Auto-merge
@sizzlemctwizzle sizzlemctwizzle removed the BLOCKING Concentrate on bug fixes primarily. label Oct 14, 2017
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Nov 22, 2017
* This sort of usage is a reason this was implemented

Post OpenUserJS#1067 and loosely related to OpenUserJS#819
Martii added a commit that referenced this issue Nov 22, 2017
* This sort of usage is a reason this was implemented

Post #1067 and loosely related to #819

Auto-merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. sooner Sooner would be appreciated.
Development

No branches or pull requests

3 participants