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

Make shared threads not require any auth #37

Merged
merged 2 commits into from
Apr 1, 2024

Conversation

waleedkadous
Copy link
Collaborator

No description provided.

This commit replaces logger.warning("Error is", e) with logger.warning(f"Error is {e}") to resolve a TypeError caused by inconsistent string formatting.
@abdullah-alnahas
Copy link
Collaborator

Thanks, Waleed, for asking me to do the review.

Seems like the "share" table is on the api_v2 branch. I suggest editing the table design as follows:

Building on Ibrahim's suggestion, I also recommend adding some metadata:

CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; 
CREATE TABLE share (
    id UUID PRIMARY KEY DEFAULT uuid_generate_v4(),
    content TEXT NOT NULL,
    shared_by INTEGER NOT NULL,
    shared_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP,
    thread_id INTEGER,
    title TEXT,
    expiration TIMESTAMP,
    FOREIGN KEY (shared_by) REFERENCES users(id),
    FOREIGN KEY (thread_id) REFERENCES threads(id)
);

What are your thoughts? @waleedkadous

@abdullah-alnahas
Copy link
Collaborator

After creating the share table in api_v2 and fixing the type error, everything works as expected. I ran the tests, and they all passed.

@abdullah-alnahas abdullah-alnahas merged commit 85f7a56 into api-v2 Apr 1, 2024
1 of 2 checks passed
@waleedkadous waleedkadous deleted the feedback_no_token branch April 1, 2024 16:12
@waleedkadous
Copy link
Collaborator Author

Thanks, Waleed, for asking me to do the review.

Seems like the "share" table is on the api_v2 branch. I suggest editing the table design as follows:

Building on Ibrahim's suggestion, I also recommend adding some metadata:

CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; 
CREATE TABLE share (
    id UUID PRIMARY KEY DEFAULT uuid_generate_v4(),
    content TEXT NOT NULL,
    shared_by INTEGER NOT NULL,
    shared_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP,
    thread_id INTEGER,
    title TEXT,
    expiration TIMESTAMP,
    FOREIGN KEY (shared_by) REFERENCES users(id),
    FOREIGN KEY (thread_id) REFERENCES threads(id)
);

What are your thoughts? @waleedkadous

I think the title makes sense, but what feature do the other fields support? I am a fan of adding only what we need. We can always add these later. Let me know?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants