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

Add Route for external tool access #1532

Closed
wants to merge 3 commits into from

Conversation

G-Ambatte
Copy link
Collaborator

This PR adds a /template route, which will allow external apps to create a Homebrewery document via URL. This is achieved by allowing text, style, title, and description to be set via parameters in the URL query.

image

image

@ericscheid
Copy link
Collaborator

What happens if ..

  1. /template route is used to create the unsaved brew
  2. some edits are made in the editor (but not yet saved)
  3. the page is reloaded?

@G-Ambatte
Copy link
Collaborator Author

This re-uses the existing NewPage so changes are saved to localStorage and can be recovered at /new (although I note that only the parts that have been edited are saved; if brew.style is not changed but brew.text is, only brew.text data will appear at /new).
However, the explicit purpose of the /template path is specifically to provide a pathway for other tools to provide data to HB, so if an external tool produces a template link, a document with the same content will be created every time that the link is followed - so refreshing the page will eliminate any changes that have been made.
It is worth noting that with current state of the commit, if there is data in localStorage (for example, from entering data on /new) that is not explicitly overwritten by the query parameters in the /template path, then the localStorage values will be used.

@ericscheid
Copy link
Collaborator

Yes. So we need to be clear on whether this route is to be used as an API (in which case it should instead be returning a json resource) or whether it should produce an user-editing environment (in which case it should alter the eventual document.location so a refresh doesn't destroy any user edits).

This is similar to the #1499 issue.

@G-Ambatte
Copy link
Collaborator Author

(in which case it should alter the eventual document.location so a refresh doesn't destroy any user edits)

I floated this idea briefly on Gitter, but what if we went in the opposite direction? Disable autosaves (with a big warning bar) so that nothing is stored in localStorage, meaning that if the user has been working on a Brew in /new then it isn't overwritten when they play with a Templated or Cloned Brew. This should also mean that if there are unsaved changes, a confirmation box will appear when they try to close the tab, warning them again that they have changes that they have not saved.
In this theoretical, there should be no user expectations that do not align with the actual results as to exactly what will happen if/when they close or refresh the tab.

Unfortunately the problem with that last sentence is the word that starts with "s".


An alternate idea along a similar line would be to lock the Editor on Cloned and Templated Brews, so the only way to unlock it for editing is to save a unique copy first (thus redirecting to the /edit page).


So we need to be clear on whether this route is to be used as an API (in which case it should instead be returning a json resource) or whether it should produce an user-editing environment (in which case it should alter the eventual document.location so a refresh doesn't destroy any user edits)

Based on previous discussions with @calculuschild, I don't believe the intention is to allow external tools to write brews to the HB database directly; but rather to allow the production of a brew which the user may then save/print/whatever at their own discretion.

@ericscheid
Copy link
Collaborator

I'm uncomfortable with disabling localStorage saving. It is there as a safety net for those brave folk that work for hours on their masterpiece before hitting the save button.

If anything, the /template and /clone (sic) routes should prompt for replacing the current /new scratchspace at initial load.

"You already have brew text in your editor, do you want to replace it?"

@ericscheid
Copy link
Collaborator

ericscheid commented Aug 9, 2021

Based on previous discussions with @calculuschild, I don't believe the intention is to allow external tools to write brews to the HB database directly; but rather to allow the production of a brew which the user may then save/print/whatever at their own discretion.

Agree, rollout of an API to facilitate 3rd-party tools to integrate deserves deeper thought.

Started a discussion: #1538

@G-Ambatte
Copy link
Collaborator Author

G-Ambatte commented Aug 10, 2021

If anything, the /template and /clone (sic) routes should prompt for replacing the current /new scratchspace at initial load.

"You already have brew text in your editor, do you want to replace it?"

I could see this working; on "Yes", update localStorage and redirect to /new. On "No", disable autosave to localStorage, so that content will not be affected and changes will only be saved if the user manually hits 'Save'.

Or, an alternate line of thought: Clones/Templates could save to a different localStorage key(s), so they do not affect the contents of /new.

<!- When I was working on #1534 last night, I used JSON.stringify to push an object to localStorage and JSON.parse to retrieve the data, specifically so it could be easily expanded on later. However, this morning I was struck by a thought: why not just push the entire this.state.brew through stringify, then write a function in getInitialState() that parses all the properties in the object retrieved from localStorage into the page's Brew? Then /new, /clone, /template and any other similar future page could all have their own localStorage key and not cross-contaminate their data. -!> Of course, this is a different scope than what this PR originally sets out to achieve.

@calculuschild calculuschild added the P3 - low priority Obscure tweak or fix for a single user label Oct 7, 2021
@calculuschild
Copy link
Member

calculuschild commented Oct 29, 2021

@G-Ambatte A problem with relying on encoding in the URL is we are limited to something like 2048 characters. That doesn't stretch very far.

We probably need to rely on using a post method so 3rd parties can send full JSON objects. I'm not 100% sure the best way to code that, but my initial guess would be to add another route in server.js to /template or whatever, have that grab the posted JSON object out of the req object (similar to what we have in homebrew.api.js):

const newBrew = (req, res)=>{
const brew = req.body;

So instead of looking up a brew object from the database by ID or anything, we already have an object provided by the 3rd party.

3rd party can then just make a button or something using a simple HTML form if they don't want to implement a javascript solution (from stackExchange):

<form method="post" action="http://example.com/example.php"
  target="_blank">
<input type="hidden" name="title" value="value1">
<input type="hidden" name="text" value="value2">
<input type="submit" value="Open results in a new window"> 
</form>

@calculuschild calculuschild marked this pull request as draft December 2, 2021 16:51
@calculuschild calculuschild added the 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure label Dec 11, 2021
@calculuschild
Copy link
Member

Closing. This needs further discussion on the underlying issue to make sure how/whether this needs to be handled going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 - low priority Obscure tweak or fix for a single user 🔍 R1 - Reviewed - Needs Discussion 💬 PR not okayed yet; needs reevaluation or closure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants