Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add /report endpoint #762

Merged
merged 4 commits into from May 4, 2016

Conversation

Projects
None yet
2 participants
Owner

erikjohnston commented May 4, 2016

No description provided.

@NegativeMjark NegativeMjark commented on the diff May 4, 2016

synapse/storage/schema/delta/32/reports.sql
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+
+CREATE TABLE event_reports(
@NegativeMjark

NegativeMjark May 4, 2016

Contributor

a timestamp and an incrementing ID might be useful here.

Contributor

NegativeMjark commented May 4, 2016

Other than adding an incrementing ID so the schema can be updated incrementally. LGTM

Owner

erikjohnston commented May 4, 2016

@NegativeMjark Like so?

@NegativeMjark NegativeMjark commented on an outdated diff May 4, 2016

synapse/storage/schema/delta/32/reports.sql
@@ -15,6 +15,8 @@
CREATE TABLE event_reports(
+ id BIGINT NOT NULL,
@NegativeMjark

NegativeMjark May 4, 2016

Contributor

Maybe make the id a primary key?

@NegativeMjark NegativeMjark and 1 other commented on an outdated diff May 4, 2016

synapse/storage/schema/delta/32/reports.sql
@@ -15,6 +15,8 @@
CREATE TABLE event_reports(
+ id BIGINT NOT NULL,
+ received_ts BIGINT NOT NULL,
@NegativeMjark

NegativeMjark May 4, 2016

Contributor

Maybe chuck an index on the received_ts? (Not the end of the world if you don't. I imagine it should be fairly easy to add one later)

@erikjohnston

erikjohnston May 4, 2016

Owner

I really don't see the benefit of adding an index we don't use? I don't even know how this data is going to be used.

Contributor

NegativeMjark commented May 4, 2016

LGTM.

We might want to only allow one report per event per reporter. but like so much else with this PR it can probably wait until later.

@erikjohnston erikjohnston merged commit 97b9141 into develop May 4, 2016

8 checks passed

Flake8 + Packaging (Commit) Build #592 origin/erikj/report_event succeeded in 33 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Postgres (Commit) Build #576 origin/erikj/report_event succeeded in 4 min 46 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #583 origin/erikj/report_event succeeded in 5 min 43 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Commit) Build #636 origin/erikj/report_event succeeded in 1 min 8 sec
Details
Unit Tests (Merged PR) Build finished.
Details

@richvdh richvdh deleted the erikj/report_event branch Dec 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment