If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add makeid to Goggles project model

RESOLVED FIXED

Status

Webmaker
X-Ray Goggles
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mjschranz, Assigned: mjschranz)

Tracking

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
We are going to need goggles projects saving the makeid for cascading deletes.
(Assignee)

Comment 1

4 years ago
Created attachment 807909 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/28

I'm not very familiar with Goggles, but as best I can tell it's nearly identical to Thimble as far as it's publishing system goes.
Attachment #807909 - Flags: review?(pomax)
Attachment #807909 - Flags: review?(jon)
Comment on attachment 807909 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/28

in fact, the code you touche here is identical between the two projects. r+
Attachment #807909 - Flags: review?(pomax) → review+

Comment 3

4 years ago
Comment on attachment 807909 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/28

r- with some nits noted in the PR. You'll need to add the new column to lib/models/thimbleproject.js, add the unique constraint to it first.
Attachment #807909 - Flags: review?(jon) → review-
(Assignee)

Comment 4

4 years ago
Comment on attachment 807909 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/28

Fixed.

Are you really sure about https://github.com/mozilla/goggles.webmaker.org/pull/28/files#r6510335? Seems to conflict with the recent problems we had with MakeAPI and contentType, although that was Mongo and not SQL.
Attachment #807909 - Flags: review- → review?(jon)

Comment 5

4 years ago
Comment on attachment 807909 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/28

r-:

* add the unique constraint to the Sequelize model
* I got a crash on lib/middleware.js:189:

ReferenceError: project is not defined
    at /Users/jon/Sites/goggles.webmaker.org/lib/middleware.js:189:25
    at null.<anonymous> (/Users/jon/Sites/goggles.webmaker.org/lib/database.js:115:37)
    at EventEmitter.emit (events.js:95:17)
    at null.<anonymous> (/Users/jon/Sites/goggles.webmaker.org/node_modules/sequelize/lib/query-interface.js:291:17)
    at EventEmitter.emit (events.js:117:20)
    at null.<anonymous> (/Users/jon/Sites/goggles.webmaker.org/node_modules/sequelize/lib/dialects/mysql/query.js:32:14)
    at Query.Sequence.end (/Users/jon/Sites/goggles.webmaker.org/node_modules/mysql/lib/protocol/sequences/Sequence.js:66:24)
    at Query._handleFinalResultPacket (/Users/jon/Sites/goggles.webmaker.org/node_modules/mysql/lib/protocol/sequences/Query.js:139:8)
    at Query.OkPacket (/Users/jon/Sites/goggles.webmaker.org/node_modules/mysql/lib/protocol/sequences/Query.js:73:10)
    at Protocol._parsePacket (/Users/jon/Sites/goggles.webmaker.org/node_modules/mysql/lib/protocol/Protocol.js:169:24)
Attachment #807909 - Flags: review?(jon) → review-
(Assignee)

Comment 6

4 years ago
Comment on attachment 807909 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/28

Oops.
Attachment #807909 - Flags: review- → review?(jon)

Comment 7

4 years ago
Comment on attachment 807909 [details] [review]
https://github.com/mozilla/goggles.webmaker.org/pull/28

r+ with nits, add "async" to the package.json file and this is ready to roll!
Attachment #807909 - Flags: review?(jon) → review+

Updated

4 years ago
Blocks: 919693

Updated

4 years ago
No longer blocks: 919693
Depends on: 919693

Comment 8

4 years ago
Matt, merge at will, but don't push to staging until :jp has changed the DB on staging okay?
(Assignee)

Comment 9

4 years ago
Landed as https://github.com/mozilla/goggles.webmaker.org/commit/708817078b7d61574c9b380b3dd96625858d506a
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.