Skip to content

Apps in an SQL DB#56

Closed
vporton wants to merge 47 commits into
apssouza22:mainfrom
vporton:feat/apps-sql
Closed

Apps in an SQL DB#56
vporton wants to merge 47 commits into
apssouza22:mainfrom
vporton:feat/apps-sql

Conversation

@vporton

@vporton vporton commented Oct 18, 2023

Copy link
Copy Markdown
Contributor

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

vporton commented Oct 18, 2023

Copy link
Copy Markdown
Contributor Author

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

vporton commented Oct 18, 2023

Copy link
Copy Markdown
Contributor Author

@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

vporton commented Oct 19, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

vporton commented Oct 21, 2023

Copy link
Copy Markdown
Contributor Author

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

vporton commented Oct 21, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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, admin@gmail.com, 123, Alex).

Before that I had to fix the create table script

CREATE TABLE users (
    id integer primary key NOT NULL,

@vporton

vporton commented Oct 22, 2023

Copy link
Copy Markdown
Contributor Author

@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="admin@gmail.com"))
            self.dao.db.conn.commit()
        except IntegrityError as e:
            print("IGNORED EXCEPTION", e)  # TODO: Remove this line.
            self.dao.db.conn.rollback()

@vporton

vporton commented Oct 23, 2023

Copy link
Copy Markdown
Contributor Author

@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

vporton commented Oct 27, 2023

Copy link
Copy Markdown
Contributor Author

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