876509 - set up two different routes for /edit vs. /remix

Status: RESOLVED FIXED

(Webmaker Graveyard :: Thimble, defect)

Description

[Pomax]

The /remix route should force any person, including the owning user, to publish a new page when they hit publish.

The /edit route should allow the owning user to edit the current page, publishing of which simply updates the DB and Make records, keeping the same URL. (if /edit is used by a non-owning user, the code acts as if /remix was used).

Comment 1

[Pomax]

Attachment: https://github.com/mozilla/thimble.webmaker.org/pull/80

This is a provisional pull request. It cannot be landed, but is necessary to get input on code choices we made. Based on the feedback I will refine the pull request for R?

Comment 2

[Scott [:thecount] Downe]

I have some pull request comments.

I think I have a theory that'll allow you to kill off one of the variables in the remixoredittemplate function.

Also would clean up that function's name.

Split it into two. You're remix path only does one thing, sets something. Can probably make that a one shot middleware function that is simply called before the edit in the cases where remix is being forced.

Comment 3

[David Humphrey (:humph)]

I'm happy to review when done, I'll let you deal with Scott's stuff first.

Comment 4

[Pomax]

Comment on attachment 754573 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/80

good suggestions, I refactored the code to set up a default pageEdit permission (although only on the /, /...remix and /...edit routes, because if it's anonymous universal it will get set on every resource request. Including our 30 JS/CSS requests, leading to the req.session.pageEdit value being "false" again since the JS/CSS loads after the actual page itself (which was funny).

Comment 5

[Jon Buckley [:jbuck]]

Why is pageEdit living on req.session? It's per-request, not for an entire session.

Comment 6

[Pomax]

it's for a session, as loading up "/edit" and hitting the publish button are ages, and about a hundred other route requests apart.

Comment 7

[Pomax]

but it gets reset based on whether you load up thimble on /, on .../remix or .../edit

Comment 8

[Scott [:thecount] Downe]

Comment on attachment 754573 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/80

This looks sane to me.

I found a single nit. r+ from me with that fix.

Comment 9

[Jon Buckley [:jbuck]]

What would be the expected behaviour when I open two tabs, one with /remix, and one with /edit? With the current code, it'll be whatever tab was loaded last will set the behaviour for the entire session.

Comment 10

[Pomax]

I'm open to suggestions on how to tackle this issue. set something in index.html during the request that is then used when the publish button's JS is called?

Comment 11

[Jon Buckley [:jbuck]]

From IRC, some context I needed:

Pomax jbuck: I'm rewriting it atm to be based on the request, setting on-page JS instead so that the 'this is an edit' info comes with the publish button's POST
jbuck Pomax: I think that will work better
jbuck but I am curious
Pomax seems safer. only tricky bit is getting the page to know it's edit:true after someone hits the button
jbuck what's the use case /edit vs /remix?
Pomax edit = you update your page
jbuck because we don't want to expose two URLs to the user right?
Pomax remix = you will publish something new if you hit publish
Pomax always, even if it's yours
brett jbuck they're what they sound like
brett so
brett if its my project, /edit is to edit/update it.  URL remains stable
jbuck right, I understand that.... but like, I don't understand how you'll expose that in a way that makes sense to the user.
Pomax /edit you only typically reach from your dashboard
Pomax /remix you reach from clicking remix button/link/whathaveyou
brett ^^ this
Pomax so if you want to update your project, you do it through webmaker.org
jbuck okay, that makes sense
Pomax but if you load up a page and hit remix, and that takes you to webmaker.org, it's going to be a new page when you publish. separation of intent.

As for how to do it, rendering a variable into the index.html based on the route makes the most sense. Then when Thimble goes to publish, it can send that "AlwaysPublishNew" variable along... or something.

Comment 12

[Pomax]

Comment on attachment 754573 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/80

reworked the code to use per-request pageOperation value (Rather than per session) and made the index.html route something that sits in routes, rather than middleware.

Comment 13

[Pomax]

Comment on attachment 754573 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/80

enough changes to require a second review pass

Comment 14

[Pomax]

Comment on attachment 754573 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/80

reassigned R? to humph

Comment 15

[Jon Buckley [:jbuck]]

Comment on attachment 754573 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/80

r+

Comment 16

[Scott [:thecount] Downe]

Comment on attachment 754573 [details] [review]
https://github.com/mozilla/thimble.webmaker.org/pull/80

R+ with a nit.

Comment 17

[Pomax]

landed.