Skip to content

Conversation

@timandrews335
Copy link

Added the ability to preserve forward slashes in cleanUpString, so we may have ingredients with fractional amounts.

Tim Andrews added 2 commits June 18, 2020 22:44
@frlan
Copy link

frlan commented Jun 20, 2020

Just wanted to open up a bug report for incredians itself .... something like »butter/margarine« ;)

@timandrews335
Copy link
Author

Just wanted to open up a bug report for incredians itself .... something like »butter/margarine« ;)

yeah, this should also take care of slashes in listed ingredients that aren't fractions. I'm not sure why slashes were stripped from it originally

@mrzapp
Copy link
Contributor

mrzapp commented Jun 23, 2020

@timandrews335 actually, although this is a nice implementation, I think it would be better to escape the slashes rather than remove them. Do you think you can alter your pull request to do that instead?

@timandrews335
Copy link
Author

@timandrews335 actually, although this is a nice implementation, I think it would be better to escape the slashes rather than remove them. Do you think you can alter your pull request to do that instead?

Yes sounds good!

@timandrews335
Copy link
Author

timandrews335 commented Jun 23, 2020

@timandrews335 actually, although this is a nice implementation, I think it would be better to escape the slashes rather than remove them. Do you think you can alter your pull request to do that instead?

Hi @mrzapp -
What I actually found is that is this is related to issue # 205, where the removal of slashes was added to cleanUpString to prevent extra subdirectories from being created. Escaping the slash didn't work for ingredients or the text of the ingredients - it actually shows "/" in the ingredients field. I think this may be related to the default behavior of encode_json(). It also makes sense for the name of the recipe to be the same as the name of the directories - and directories cannot have slashes. Therefore, I changed my optional parameter in cleanUpString to be $remove_slashes (instead of $preserve_slashes). The default would be to keep the slashes in the string. Passing true is now done when the name is cleaned up. Long story short - everything will keep the slash except the name. I hope that works. For a future enhancement, I think we could have separate fields and logic for the name that represents the directory structure and the friendly name that would show in the recipe's title.

@mrzapp
Copy link
Contributor

mrzapp commented Jun 23, 2020

@timandrews335 great, thanks for the contribution! One last check, we had a report of categories and keywords breaking when they had slashes in them, is that fixed with this pull request too?

@timandrews335
Copy link
Author

@timandrews335 great, thanks for the contribution! One last check, we had a report of categories and keywords breaking when they had slashes in them, is that fixed with this pull request too?

@mrzapp do you know what issue that was (is it an open or closed one?). I just edited a category and a keyword, put in a slash, and it worked. By breaking, did they they mean categories and keywords were removing slashes? If that's the case, this should fix that.

@mrzapp mrzapp merged commit d658596 into nextcloud:master Jun 25, 2020
@mrzapp
Copy link
Contributor

mrzapp commented Jun 25, 2020

@timandrews335 alright, if you tested it without issues, then I'll merge it :) Thanks again for the contribution

@mrzapp
Copy link
Contributor

mrzapp commented Jun 25, 2020

@timandrews335 I'm seeing the issue now, if you add a slash to a category and then click it, I get a "page not found" error, and the JavaScript crashes.

@sam-19 any ideas?

@sam-19
Copy link
Contributor

sam-19 commented Jun 25, 2020

Sorry, my router suffered a catastrophic breakdown and I've been kind of in the dark waiting for a replacement for the past week.

This problem is caused by the fact that forward slashes are preserved key characters for URL route matching schemes (same as in file system paths). Using a slash in a parameter in the URL will break route matching. So it has to either be escaped or removed. No other way around it that I know of, sorry.

@mrzapp
Copy link
Contributor

mrzapp commented Jun 26, 2020

Ok, I'll remove slashes specifically for categories, then

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants