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

Apps in an SQL DB #56

Closed
wants to merge 47 commits into from
Closed

Apps in an SQL DB #56

wants to merge 47 commits into from

Conversation

vporton
Copy link
Contributor

@vporton vporton commented Oct 18, 2023

Intended to fix issue #7.

This is a work in progress. I created this PR to track its dependency:

Depends on PR #55.

I just write [WIP] in the title instead of creating a draft pull request, because I had some awkward experience with draft pull request, something about having a hardship to convert it to a normal PR.

@vporton
Copy link
Contributor Author

vporton commented Oct 18, 2023

I've implemented it but with two BUTS:

  1. not at all tested
  2. it does not use PK to query objects, as it should in clean code

I am not sure whether to test it now or first solve "2".

@vporton
Copy link
Contributor Author

vporton commented Oct 18, 2023

@apssouza22 I have a trouble finding Python code that loads source_data.json to the DB (that was a file DB in old code).

I noticed the file prepare_data.py but don't see in this file storing data in the file DB.

Please, help to find this code or understand why it is missing.

@vporton vporton changed the title [WIP] Apps in an SQL DB Apps in an SQL DB Oct 19, 2023
@vporton
Copy link
Contributor Author

vporton commented Oct 19, 2023

Now create app and list apps work.

I propose you @apssouza22 to merge this PR to main, because so we will find the rest bugs, if any, during working with main branch.

@vporton vporton mentioned this pull request Oct 19, 2023
@apssouza22
Copy link
Owner

@vporton it is hard to test all these changes. Could you remove Alckemy from this MR? Make it simple. Addressing only one thing(store apps in the database)

On the opensource world we need to make PRs very easy small and easy to follow because mantainers don't have time to dig into big changes

@vporton
Copy link
Contributor Author

vporton commented Oct 21, 2023

it is hard to test all these changes. Could you remove Alckemy from this MR? Make it simple.

It's not very easy to remove Alchemy from this set of changes because of subtle differences, but well I will do.

@vporton
Copy link
Contributor Author

vporton commented Oct 21, 2023

I removed SQLAlchemy code.

Please, accept this PR. It may contain errors, but in the present time the main method of software assurance is testing, not mathematical proof that it's correct. So, we will have enough time to test it, when it will be in the main branch, because our project is still unfinished.

@vporton vporton mentioned this pull request Oct 21, 2023
@apssouza22
Copy link
Owner

@vporton the app works fine today. We can not merge anything that breaks it.

I have pulled your branch the app is not even starting. Please do some basic taste before open the PR.

chatflow-postgres-1  | 2023-10-22 19:53:14.690 UTC [60] DETAIL:  Failing row contains (null, [email protected], 123, Alex).

Before that I had to fix the create table script

CREATE TABLE users (
    id integer primary key NOT NULL,

@vporton
Copy link
Contributor Author

vporton commented Oct 22, 2023

@apssouza22 Please, create branch develop to integrate there changes that were not yet tested (instead of handing multiple non-merged pull requests).

Fixed table users.

This error can be safely ignored. I tried to handle it with except: but in some reason it is not caught:

        try:
            self.add_user(User(name="Alex", password="123", email="[email protected]"))
            self.dao.db.conn.commit()
        except IntegrityError as e:
            print("IGNORED EXCEPTION", e)  # TODO: Remove this line.
            self.dao.db.conn.rollback()

@vporton
Copy link
Contributor Author

vporton commented Oct 23, 2023

@apssouza22 Oh, sorry, there really was a bug with DETAIL: Failing row contains. It is because of a wrong SQL code, and I did not notice it, because I already executed correct code in the past. Now it is fixed.

@vporton
Copy link
Contributor Author

vporton commented Oct 27, 2023

Superseded by PR #62.

@vporton vporton closed this Oct 27, 2023
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