876522 - Implement /edit and /remix routes for Popcorn Maker

Status: RESOLVED FIXED

(Webmaker Graveyard :: Popcorn Maker, defect)

Description

[Brett Gaylor [:brett]]

In thimble, we're implementing the above routes in bug #876509.  We'll need to implement this in Popcorn Maker as well.

Comment 1

[Matthew Schranz [:mjschranz]]

So is bug 876457 no longer valid?

Comment 2

[Scott [:thecount] Downe]

I think this ticket is for the difference between edit and remix. They are not the same. This also is for this to work on the tool level.

bug 876457 to me is creating these paths on the user domain level, which just redirects to the tool level, thus it'll end up redirecting to the work done in this bug.

Comment 3

[Scott [:thecount] Downe]

The way popcorn does it right now is "http://localhost:8888/templates/basic/?savedDataUrl=/api/remix/30"

Is it the goal to completely change that to "http://localhost:8888/30/remix", or just update the remix part in there, to also have an edit?

Basically, some url ideas being thrown down. I need some help figuring out where we're going with this. What do we want the tool level url to look like.

"http://localhost:8888/templates/basic/?savedDataUrl=/api/remix/30" // this is current
"http://localhost:8888/templates/basic/?savedDataUrl=/api/edit/30"
"http://localhost:8888/templates/basic/?savedDataUrl=/api/30/remix"
"http://localhost:8888/templates/basic/?savedDataUrl=/api/30/edit"
"http://localhost:8888/30/remix" // super simple
"http://localhost:8888/30/edit"
"http://localhost:8888/templates/basic/30/remix"
"http://localhost:8888/templates/basic/30/edit"

Or something inbetween?

Anything that doesn't use the ?savedDataUrl is much more work, but doable.

Comment 4

[Jon Buckley [:jbuck]]

I'm totally down with getting rid of templates/basic/ structure and savedDataUrl, but it will be a ton of work. At the very least you'll need to convert templates/basic/index.html to a nunjucks (or jade :/) template. To be honest, it's something that could easily take a week.

Comment 5

[Jon Buckley [:jbuck]]

So, because of that, don't implement any new code is basically what I'm saying. Aim to do that after June 15th.

Comment 6

[Scott [:thecount] Downe]

Attachment: https://github.com/mozilla/popcorn.webmaker.org/pull/24

I actually think this is doable from here. I think we can get this in a reasonable amount of time. Couple days, not a week.

I've got it in a place where it is ready for review.

Might need to change the url we use. To avoid bikshedding the url, I went with:
http://localhost:8888/basic/id/remix
http://localhost:8888/basic/id/edit

I was able to plug into
/api/remix/id
/api/project/id

I am sure there are bits that while good to have, can wait until after the 15th. Landing the core functionality though we should do now.

Comment 7

[Matthew Schranz [:mjschranz]]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

Lot's of nits. You also never added nunjucks to the package.json so I had to install it myself for this all to work.

Also, while I don't think this needs to be done here, do we have something filed to have our publish urls structure themselves similarly to how Thimble does? I know with Thimble I could throw a /edit on the URL I got before and was magically in the editor.

Comment 8

[Scott [:thecount] Downe]

"with Thimble I could throw a /edit on the URL I got before and was magically in the editor"

That's what bug 876457 is for, I believe.

Comment 9

[David Humphrey (:humph)]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

Please finish with Matt's review before requesting more.  I can look at this once you've got an r+ from him.

Comment 10

[Matthew Schranz [:mjschranz]]

Good point brought up by humph in https://bugzilla.mozilla.org/show_bug.cgi?id=876440#c11. I don't know if should worry about this in this ticket but we definitely need to factor this in.

Comment 11

[Scott [:thecount] Downe]

I cleared up bug 876457's description and title.

My intent on that ticket is being lost. I tried to clear it up. Let me know if it doesn't sound like what you guys are requesting. There might still be confusion.

This ticket is for the app level url, and bug bug 876457 is for the S3 user domain level url.

I don't think we should do that in this ticket.

Comment 12

[Scott [:thecount] Downe]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

Added the details page as per master into nunjucks.

Fixed up or commented on all your review pull request comments.

Comment 13

[Matthew Schranz [:mjschranz]]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

I had one big additional question in the comments.

Also, you still didn't add nunjucks as a dependency in the package.json. I'd throw this to Jon afterwards since he's back now.

Comment 14

[Scott [:thecount] Downe]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

Updated. Remembered to update package.json this time.

Also, regarding displaying a project if a user is not logged in. This is safe to do. Or, we should ensure it is safe to do.

It is not until we hit publish that we need to be logged in, and owner of the project. If we are logged in and do not own the project, it should remix instead.

Assigning back to Matt to confirm on that.

Also, by the sounds of Matt's last comment, he's ready to have Jon look at this.

Comment 15

[Matthew Schranz [:mjschranz]]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

I'm fine with it all at this point.

We definitely will be handling the S3/local file storage linking in https://bugzilla.mozilla.org/show_bug.cgi?id=876457

Comment 16

[Jon Buckley [:jbuck]]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

r-, left a bunch of comments in the PR. Is there any way you can split this up into multiple smaller patches? Maybe start with just the nunjucks -> jade conversion?

Comment 17

[Scott [:thecount] Downe]

I can split it.

Comment 18

[Scott [:thecount] Downe]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

Updated this after the split.

Same pull request.

Comment 19

[Jon Buckley [:jbuck]]

Comment on attachment 755075 [details] [review]
https://github.com/mozilla/popcorn.webmaker.org/pull/24

r+ with nits noted in the pull request.

Comment 20

[Scott [:thecount] Downe]

Staged: https://github.com/mozilla/popcorn.webmaker.org/commit/34717bc912d1e0776dc21d1b7a27cc5ea076a360