search

overhangio / tutor-ecommerce

Ecommerce plugin for Tutor
GNU Affero General Public License v3.0
17 stars 50 forks source link

fix: allow multi-line values in ECOMMERCE_PAYMENT_PROCESSORS #32

Closed lpm0073 closed 1 year ago

lpm0073 commented 2 years ago

… }}, strict=False) to allow multiline content

regisb commented 2 years ago

I'm not sure I understand the impact of this change. Could you please give more information?

lpm0073 commented 2 years ago
Screen Shot 2022-07-26 at 8 38 57
regisb commented 2 years ago

Nope, I still don't get it. You are showing a sample from a YAML file but the code extracts values from JSON. I do not understand what issue the proposed change is supposed to fix.

regisb commented 2 years ago

@lpm0073 Can you please give more details about what you are trying to achieve here?

regisb commented 2 years ago

I will close this because of the lack of response. If you want to re-open, please describe what issue this is supposed to fix.

lpm0073 commented 2 years ago

hi @regisb, not sure what additional information i can provide, but, closing the PR didn't help anyone; least of all the repo maintainers here. recasting my original text from above: if the value of apple_pay_merchant_id_domain_association contains a CR/LF then the code will raise an exception. that's it, really. there's more technical info here about the exact nature of my 10-character PR: https://docs.python.org/3/library/json.html. of note:

If strict is false (True is the default), then control characters will be allowed inside strings. Control characters in this context are those with character codes in the 0–31 range, including '\t' (tab), '\n', '\r' and '\0'.

i worked around this btw, and so this bug doesn't block me in any way, but it'd be great if the fix found its way into the master branch at some point. thanks :)

regisb commented 2 years ago

if the value of apple_pay_merchant_id_domain_association contains a CR/LF then the code will raise an exception

This is exactly the information that I needed in order to understand the purpose of this PR. With this information I am now able to reproduce the issue. To be quite thorough you should have mentioned that the raised error is JSONDecodeError: Invalid control character at: line 1 column xx (char xx).

With this in mind, I'd like to suggest a different fix. The current implementation dumps ECOMMERCE_PAYMENT_PROCESSORS to json before loading it back again, which is quite useless. Instead, could we simply write:

common_payment_processor_config = {{ ECOMMERCE_PAYMENT_PROCESSORS }}

Could you please test this change @lpm0073?

More generally, when I look at an issue report, I need at least two things:

  1. A detailed description of what problem is occurring. Here, it should be: "when the ECOMMERCE_PAYMENT_PROCESSORS setting contains multi-line values, running tutor local start triggers a JSONDecodeError".
  2. How to reproduce the issue. Something along the lines of: "1. enable the ecommerce plugin 2. edit config.yml to add a multi-line entry to ECOMMERCE_PAYMENT_PROCESSORS 3. run tutor local start".

Without these two elements it may be really difficult for me to guess what is going on, especially with a complex plugin such as ecommerce, which I use as seldom as possible...

Also, I closed the PR because you did not reply to either my July 26 or September 8th comments. I think it's fair to close the PR in such cases, as I try to keep the list of open PRs/issues as short as possible at all times. Also, there is always the possibility of re-opening the PR, as I just did.

regisb commented 2 years ago

@lpm0073 Did you see my comment above? What are your thoughts on the proposed change?