Remove short ID #2148

TheSlimvReal · 2024-01-02T08:23:53Z

closes: #1526

(fully?) remove usage of short ID
adjust PouchDB views
URL supports both ways, using short ID though
~~change reference to enum in additional property of schema~~
remove addPrefix query function and toEntities(...)' prefix variable - REQUIRES MIGRATION

github-actions · 2024-01-02T08:24:12Z

Deployed to https://pr-2148.aam-digital.net/

sleidig

this looks pretty good to me!
Not sure how far the indexing still has problems or what tasks remain open?

Not functionally tested yet.

src/app/child-dev-project/children/children.service.ts

sleidig · 2024-01-03T14:38:02Z

src/app/child-dev-project/children/children.service.ts

-            emit(["${School.ENTITY_TYPE}:" + doc.schoolId, start]);
+            emit([doc.childId, start]);


which needs to come first: the migration of data or the update of these index definitions? would these indices still work in old or new form with both migrated and previous data?

If we update and migrate at the same time, the user will most likely get the data before getting the app update which means it doesn't matter if we support both formats here.
On the other side I don't think we can make the code work with both the new and old data format as this is exactly why we decided for the migration.

sleidig · 2024-01-03T14:43:28Z

src/app/core/basic-datatypes/configurable-enum/configurable-enum.service.ts

+    // TODO or de we rather want to pass full IDs around here as well?
    const entityId = Entity.createPrefixedId(ConfigurableEnum.ENTITY_TYPE, id);


good question ... would be redundant, as the dataType already makes it clear. But if that makes it much easier to implement ...?

At the moment it wouldn't add a benefit to migrate it. But the question is more if we want to keep it consistent (full ID whenever possible)

sleidig · 2024-01-03T14:45:21Z

...ore/basic-datatypes/configurable-enum/configure-enum-popup/configure-enum-popup.component.ts

+      // TODO should this be migrated?
      const schemaFields = [...entity.schema.entries()]
        .filter(
          ([_, schema]) =>
-            schema.innerDataType === this.enumEntity.getId() ||
-            schema.additional === this.enumEntity.getId(),
+            schema.innerDataType === this.enumEntity.getId(true) ||
+            schema.additional === this.enumEntity.getId(true),


yes, maybe. Could be a point when we introduce a dataType for enum-multi/-array (#2121)

Do you mean a select field where values from multiple configurable enums can be selected?

src/app/core/basic-datatypes/entity/display-entity/display-entity.component.ts

src/app/core/entity-details/entity-details/entity-details.component.spec.ts

src/app/core/entity-details/entity-details/entity-details.component.ts

sleidig · 2024-01-03T14:55:27Z

src/app/core/entity/model/entity.ts

-  public getId(withPrefix: boolean = false): string {
-    if (withPrefix) return this._id;
-    return this.entityId;
+  public getId(withoutPrefix = false): string {


I would prefer to switch the default value instead of the meaning of the param (i.e. getId(withPrefix = true)) - at least for people who have used the previous system this seems the safer, less confusing way?

But this introduces the problem that the check if (withPrefix) would be false if no argument is passed. So I think the saver and generally more common approach would be that passing no argument or passing a false argument results in the same output.

Yes, that would mean not passing the param is different from passing false - but if the expected standard is to get it with prefix and explicitly passing false disables that, in my opinion that is as explicit as the other way round.
I am not insisting on this though - it mostly confused me during review here but I'll get used to the new way quickly.

src/app/core/export/data-transformation-service/data-transformation.service.spec.ts

sleidig

Looks good to me now!

I did a general walkthrough of the pr deployed app, looking at references and lists. Didn't notice any issues there functionally.

I guess the last open point is whether we do something special for aligning update + migration. And some test still failing?

# Conflicts: # src/app/child-dev-project/schools/child-school-overview/child-school-overview.component.ts # src/app/core/common-components/entity-form/entity-form.service.spec.ts # src/app/core/common-components/entity-subrecord/entity-subrecord/entity-subrecord.component.spec.ts # src/app/core/common-components/entity-subrecord/entity-subrecord/entity-subrecord.component.ts # src/app/core/filter/filters/filters.ts

sleidig · 2024-01-22T10:31:13Z

Tested deployed version on dev.aam-digital.net with the migration from Aam-Digital/ndb-admin#18 that you already applied:

on previous version (3.29.0) no errors or complete breakdowns happen, only the related-entities tables remain empty. I think that is a good and smooth experience for users during the upgrade.
upon deploying the pre-release, the app did reload automatically to load the new version, as expected
with the version from this PR here, related entities are displayed again as expected and creating new entries also works
Education History (from Child + School)
ASER
Matching
Attendance
(!) Todos: Cannot read properties of undefined (reading 'completedBy' ...
(!) Notes + Todos: cannot create new from tab -> '+' button not working in TodosRelatedToEntity and NotesRelatedToEntity #2188
Health (only updates after reload --> bug in master)
Educational Material
Observations (only updates after reload --> bug in master)

I'll look into the "update only on reload" issue separately.

Except the Notes/Todos bug things seem to work very well. (some unit test failing, however)

# Conflicts: # src/app/child-dev-project/notes/notes-related-to-entity/notes-related-to-entity.component.ts # src/app/features/todos/todos-related-to-entity/todos-related-to-entity.component.ts

aam-digital-ci · 2024-02-01T12:32:00Z

🎉 This PR is included in version 3.31.0-master.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

aam-digital-ci · 2024-02-01T12:49:31Z

🎉 This PR is included in version 3.31.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

TheSlimvReal added 2 commits January 2, 2024 09:14

reduced usage of short id in code

8a737ea

fixed some bugs

e352f99

TheSlimvReal added 7 commits January 2, 2024 09:31

fixed view issues

c2e8a9d

fixed tests

212f68d

removed furhter usages of short id

fed3556

removed furhter usages of short id in tests

4197c8e

flipped function attribute meaning of getId()

c518c43

Merge branch 'master' into remove_short_id

86c4db0

small cleanups

f4757c2

sleidig reviewed Jan 3, 2024

View reviewed changes

TheSlimvReal added 3 commits January 6, 2024 17:21

removed trivial function

44e5df0

entity details support full ids

f972732

Merge branch 'master' into remove_short_id

e3d1418

sleidig approved these changes Jan 8, 2024

View reviewed changes

TheSlimvReal and others added 8 commits January 16, 2024 09:59

added datatype config for educational material

3ff33f1

fixed showInactive logic in ChildSchoolOverview

ab6021a

removed some todos

40c67a6

todo completion displays entity toString

6e1b239

report title is optional

a08b5b5

removed addPrefix function from query service

294b3d3

Merge branch 'master' into remove_short_id

4701c92

TheSlimvReal marked this pull request as ready for review January 22, 2024 10:49

TheSlimvReal added 5 commits January 23, 2024 08:53

Merge branch 'master' into remove_short_id

7e8848d

fixed completedBy issue

250bf8f

removed duplicate test

4c2f3c9

review cleanups

6ff37db

Merge branch 'master' into remove_short_id

ed70ebb

# Conflicts: # src/app/child-dev-project/notes/notes-related-to-entity/notes-related-to-entity.component.ts # src/app/features/todos/todos-related-to-entity/todos-related-to-entity.component.ts

TheSlimvReal merged commit f86ca9d into master Feb 1, 2024
6 of 7 checks passed

TheSlimvReal deleted the remove_short_id branch February 1, 2024 12:30

aam-digital-ci added the released on @master managed by CI (semantic-release) label Feb 1, 2024

aam-digital-ci added the released managed by CI (semantic-release) label Feb 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove short ID #2148

Remove short ID #2148

TheSlimvReal commented Jan 2, 2024 •

edited

Loading

github-actions bot commented Jan 2, 2024

sleidig left a comment

sleidig Jan 3, 2024

TheSlimvReal Jan 6, 2024

sleidig Jan 3, 2024

TheSlimvReal Jan 6, 2024

sleidig Jan 3, 2024

TheSlimvReal Jan 6, 2024

sleidig Jan 3, 2024

TheSlimvReal Jan 6, 2024

sleidig Jan 8, 2024

sleidig left a comment •

edited

Loading

sleidig commented Jan 22, 2024 •

edited by TheSlimvReal

Loading

aam-digital-ci commented Feb 1, 2024

aam-digital-ci commented Feb 1, 2024

		emit(["${School.ENTITY_TYPE}:" + doc.schoolId, start]);
		emit([doc.childId, start]);

		// TODO or de we rather want to pass full IDs around here as well?
		const entityId = Entity.createPrefixedId(ConfigurableEnum.ENTITY_TYPE, id);

Remove short ID #2148

Remove short ID #2148

Conversation

TheSlimvReal commented Jan 2, 2024 • edited Loading

github-actions bot commented Jan 2, 2024

sleidig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sleidig left a comment • edited Loading

Choose a reason for hiding this comment

sleidig commented Jan 22, 2024 • edited by TheSlimvReal Loading

aam-digital-ci commented Feb 1, 2024

aam-digital-ci commented Feb 1, 2024

TheSlimvReal commented Jan 2, 2024 •

edited

Loading

sleidig left a comment •

edited

Loading

sleidig commented Jan 22, 2024 •

edited by TheSlimvReal

Loading