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.

Sign up for GitHub

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

Jump to bottom

#27 and #34- store each suggestion with unique id and alter sentences table - Appolin Semegni Fotso #38

Merged
merged 12 commits into from
Oct 12, 2023
Merged

#27 and #34- store each suggestion with unique id and alter sentences table - Appolin Semegni Fotso #38

merged 12 commits into from
Oct 12, 2023

Conversation

AppolinFotso
Copy link
Contributor

@AppolinFotso AppolinFotso commented Oct 11, 2023 •
edited
Loading

Naming Rules

Name your PR like this: ISSUENUMBER-TITLE-YOURNAME

Description

I parsed the body of the fetch POST API into JSON so that each suggestion can be stored with its unique id when inserting them into the suggestions table. I added a source and a count columns to the sentences table and I inserted the hard-coded array of text into the sentences table so that random text can be query from the database. I added appropriate queries to endpoints in api.js file.

Related to

Make sure you include the issue number with a hash sign # so Github can link this PR to the right issue, like this:

Fixes #27 and #34

Checklist:

  • My code follows the style guidelines of this project
  • I have carefully reviewed my own code
  • I have commented my code
  • I have updated any documentation

@AppolinFotso
Refactored a new submitButton function and passed it as props to PopU…
8fa17e9 
…pAlert component
@AppolinFotso
Added useState to SubmitSuggestion component to handle message after …
223fac6 
…data is posted
@AppolinFotso
Passed message props to PopUpAlert to display the return message from…
712a869 
… the server
@AppolinFotso
Added callback function to onClick in PopUpAlert
de1c0fb
@AppolinFotso
Added tenary operator in PopUpAlert to handle the return message from…
d319965 
… the server
@AppolinFotso
Added Alert component to handle unsaved suggestion
f0a3c27
@AppolinFotso
Replaced URLSearchParams with jsonData in SubmitSuggestion component
90b5bc2
@AppolinFotso
Added a for loop to store each suggestion with their unique id
c13eea7
@AppolinFotso
Updated the database schema
b6bb906
@AppolinFotso
Removed the hardcoded text on the endpoint /api and added a query to …
0a2930f 
…fetch randomtext from the database
@AppolinFotso
Added query to retrieve the sentence id from sentences table and stor…
15ead82 
…e it in suggestion and user_interaction tables
@render
Copy link

render bot commented Oct 11, 2023

Your Render PR Server URL is https://gaelic-project-pr-38.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-ckjd18sl4vmc73akpka0.

@AppolinFotso AppolinFotso changed the title #- store each suggestion with unique id and alter sentences table - Appolin Semegni Fotso #38- store each suggestion with unique id and alter sentences table - Appolin Semegni Fotso Oct 11, 2023
@AppolinFotso AppolinFotso changed the title #38- store each suggestion with unique id and alter sentences table - Appolin Semegni Fotso #27 and #34- store each suggestion with unique id and alter sentences table - Appolin Semegni Fotso Oct 11, 2023
This was referenced Oct 11, 2023
Open
Open
sztupy
sztupy reviewed Oct 11, 2023
View reviewed changes
server/db-schema.md
sentence_id INTEGER REFERENCES sentences(id),
selected_suggestion TEXT,
user_provided_suggestion TEXT
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whenever you update the schema it is also customary to provide update scripts - otherwise people will need to delete and re-create the database from scratch. This is not a problem right now, but could become an issue in the future once the project is live

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we should provide some test data - a couple of INSERT INTOs of the existing sentence data for example to make sure we have enough data to start with. Something like

INSERT INTO sentences (sentence) VALUES ('Tha iad ris an Gearrloch fhathast.');

sztupy
sztupy reviewed Oct 11, 2023
View reviewed changes
server/api.js Outdated Show resolved Hide resolved
@sztupy
Copy link
Contributor

sztupy commented Oct 12, 2023

While there are some issues with this PR I will commit it as it includes a couple important architectural changes that should be in main. The ticket should be kept open to fix a couple remaining issues as a new PR:

  1. We still need a good database setup script. We have the schema file now, but it is not runnable (if you just pass it into postgres it will throw syntax errors), and is missing setup of the seed data (the initial sentences)

  2. We should be using IDs throughout. There are two places where this becomes important:

  • In the selected_suggestion column we still store the full suggestion. This should be the id of the relevant entry in thesuggestion table
  • We send the full sentence to the frontend but not it's ID. Then, when we click a suggestion we send the full sentence back and ask the Database to find it's ID. What we should do is send both the sentence AND it's ID to the frontend, and the frontend should send BOTH back. The database can then look up the sentence by ID (and verify it is correct by matching it to the sentence). This will be still safe, but also performant. (Note that sending back the full sentence is not actually technically necessary, the ID is enough for most purposes)
  1. When getting a sentence from the database we query the entirety of the database and selecting a random one from that. Postgres is clever enough to have features (please have a look on your own) to provide you with a single random sentence - use that feature. The current implementation would lose performance once the sentence database gets huge.

@sztupy sztupy merged commit eaf8fde into main Oct 12, 2023
@AppolinFotso AppolinFotso mentioned this pull request Oct 18, 2023
Merged
4 tasks
sztupy added a commit that referenced this pull request Oct 19, 2023
@sztupy
Merge pull request #54 from gla6-fp-team1/DB-1_database-septup_and_qu…
e654882 
…ery-performance

#38 - Query performance and Database setup script - Appolin Semegni Fotso
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sztupy sztupy sztupy left review comments

@ali-nasir-ali ali-nasir-ali Awaiting requested review from ali-nasir-ali

@Melesegithub Melesegithub Awaiting requested review from Melesegithub

@filkovskyy filkovskyy Awaiting requested review from filkovskyy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@AppolinFotso @sztupy