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 some bugs with encoding #900

Closed

Conversation

Martii
Copy link
Member

@Martii Martii commented 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 Follow GH example for component URI encoding on branch name #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 may show up as / (or a list bullet) Array typed values write themselves... interesting.
  • Followup with discussion.path fixes. (ready)
  • 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
  • Not all User urls are covered in this PR (perhaps yet)

Applies to #819 and #262... treads on #262 (comment) among many others.

* 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 Martii added bug You've guessed it... this means a bug is reported. enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. DB Pertains inclusively to the Database operations. PR READY This is used to indicate that a pull request (PR) is ready for evaluation. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. labels Feb 20, 2016
@Martii Martii added migration Use this to indicate that it may apply to an existing or announced migration. and removed PR READY This is used to indicate that a pull request (PR) is ready for evaluation. labels Feb 20, 2016
@Martii
Copy link
Member Author

Martii commented Feb 21, 2016

ON HOLD... may have a fourth option that's a little clearer and retains some original functionality.

@Martii
Copy link
Member Author

Martii commented Feb 21, 2016

Abandoning for a newer PR... it should preserve more... so far so good... about 90% tested and 85% code transformed... within current code styling.

@Martii Martii closed this Feb 21, 2016
@Martii Martii added the wontfix Nope, nada, zip, zilch. label Feb 22, 2016
@Martii Martii deleted the Issue-819constantEncodingUrls branch February 26, 2016 10:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 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. enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. migration Use this to indicate that it may apply to an existing or announced migration. wontfix Nope, nada, zip, zilch.
Development

Successfully merging this pull request may close these issues.

1 participant