Page MenuHomeDevCentral

Implement basic MVC architecture
ClosedPublic

Authored by inidal on Jun 6 2023, 05:48.
Tags
None
Referenced Files
F3762454: D3182.id.diff
Thu, Nov 21, 11:42
Unknown Object (File)
Tue, Nov 19, 16:15
Unknown Object (File)
Sun, Nov 17, 18:04
Unknown Object (File)
Sun, Nov 17, 17:04
Unknown Object (File)
Sun, Nov 17, 02:59
Unknown Object (File)
Sun, Nov 17, 01:13
Unknown Object (File)
Sat, Nov 16, 23:36
Unknown Object (File)
Sat, Nov 16, 23:29
Subscribers

Diff Detail

Repository
rSP ServPulse
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

inidal requested review of this revision.Jun 6 2023, 05:48
inidal created this revision.
dereckson requested changes to this revision.Jun 6 2023, 16:39
dereckson added a subscriber: dereckson.
dereckson added inline comments.
backend/servpulse-backend/app.js
1 ↗(On Diff #8109)

Not sure why each file start by it's name

If you wish a proper header you can use this:

/*  -------------------------------------------------------------
    ServPulse :: app
    - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
    Project:        Nasqueron
    Description:    Bootstrap the application
    License:        BSD-2-Clause
    -------------------------------------------------------------    */
2 ↗(On Diff #8109)

Document in the README something like

Code conventions

The project adheres to the following code conventions:
https://agora.nasqueron.org/Code_conventions

In addition:

  • the project uses single quotes
  • both variables and functions names are in camelCase
backend/servpulse-backend/controllers/incidentController.js
2 ↗(On Diff #8109)

Can we've a utility method requireModel so we we can write requireModel('incidentModel')?

16 ↗(On Diff #8109)

? (see below)

backend/servpulse-backend/controllers/serviceController.js
24 ↗(On Diff #8109)

?

Don't commit intents, document them somewhere on a task if really needed

serviceController: methods to add after getServices (around line 24)

backend/servpulse-backend/models/serviceModel.js
5 ↗(On Diff #8109)

Need to find a way to write those queries on several lines.

Ideally:

INSERT INTO service
(name, "group", description, status)
VALUES (\$1, \$2, \$3, \$4)
RETURNING *
backend/servpulse-backend/package.json
10 ↗(On Diff #8109)

You wanted MIT fist.

But probably best to just pick BSD-2-Clause, it's virtually the same,
and will so be aligned with all other projects we have.

backend/servpulse-backend/routes/incidentRoutes.js
8 ↗(On Diff #8109)

? (see above)

backend/servpulse-backend/routes/serviceRoutes.js
9 ↗(On Diff #8109)

? (see above)

database/README.md
1 ↗(On Diff #8109)

Markdown headings:

4 ↗(On Diff #8109)
database/scripts/populate_data.sql
1 ↗(On Diff #8109)

If there are part of integration tests, put them in tests/

If there are part of an example or an help for developers, put them in docs/

If not intended to be used for development, don't commit it

(personally I'd put that somewhere like database/docs/sample_data.sql
and document in the README "If you need sample data to work in the project, use database/docs/sample_data.sql")

11 ↗(On Diff #8109)

I prefer what you had on your laptop: you had real service names

Also, you don't have any warranty first group will always be "1", so probably best to put back the id in service_group

database/servpulse.sql
1 ↗(On Diff #8109)

This folder needs to be organized as migrations, so when the schema change, we can cope with it, as the schema is versioned:

  • first migration file is this one
  • new migrations will do ALTER TABLE to modify schema
49 ↗(On Diff #8109)

Consider not to hide foreign keys by putting this in CREATE TABLE directly:

"group" int REFERENCES "service_group" ("id")
This revision now requires changes to proceed.Jun 6 2023, 16:39
  • Update main file header
  • Add code conventions
  • Update license to BSD-2
  • Remove unwanted comments
  • Correct markdown syntax
  • Remove unwanted files

Thank you for your valuable insights. I believe I have addressed most of the issues you raised. Please let me know if there is anything else I can do before moving to the front-end. Thanks again for your help!

backend/servpulse-backend/controllers/incidentController.js
2 ↗(On Diff #8109)

We can have it as a ticket for now and see how it'll unfold in the future. Let me know what you think about it.

database/scripts/populate_data.sql
1 ↗(On Diff #8109)

I don't know yet how it'll be organized and what will be useful for the next developer that'll maintain it. So, in order to avoid complexity I am deleting that file.

database/servpulse.sql
1 ↗(On Diff #8109)

I am deleting that file too.

We can have it as a ticket for now and see how it'll unfold in the future. Let me know what you think about it.

Sure.

This revision is now accepted and ready to land.Jun 13 2023, 21:39