[TTAHUB-955] Generic Report #1523
Conversation
src/migrations/20230807000000-genericReport-migrateTrainingReports.js
Outdated
Show resolved
Hide resolved
src/migrations/20230807000000-genericReport-migrateTrainingReports.js
Outdated
Show resolved
Hide resolved
src/migrations/20230807000000-genericReport-migrateTrainingReports.js
Outdated
Show resolved
Hide resolved
src/migrations/20230807000000-genericReport-migrateTrainingReports.js
Outdated
Show resolved
Hide resolved
src/migrations/20230807000000-genericReport-migrateTrainingReports.js
Outdated
Show resolved
Hide resolved
| JOIN "ValidFor" v | ||
| ON s."validForId" = v.id | ||
| AND v.name = 'collaborator' | ||
| WHERE s.name = 'approved' -- TODO: this is terminal, which could lock editing, but the others statuses make even less sense for events |
There was a problem hiding this comment.
@hardwarehuman The collaborator statusId should be left null for any non-approving user
| @@ -305,6 +305,8 @@ | |||
| "csv-parse": "^4.14.1", | |||
| "csv-stringify": "^5.6.2", | |||
| "cucumber-html-reporter": "^5.2.0", | |||
| "deepmerge": "^4.3.1", | |||
| "dot-wild": "^3.0.1", | |||
There was a problem hiding this comment.
dot-wild is a little worrying to me, there are security related issues on the repo and it hasn't been updated in 6 years
There was a problem hiding this comment.
A few more files down, I see it would be difficult to replace as it seems to be deeply intertwined with the approach we took here
There was a problem hiding this comment.
https://security.snyk.io/package/npm/dot-wild I don't see any vulnerabilities. What issues do you see?
There was a problem hiding this comment.
It had not been updated in 6 years, agreed. But it works great.
There was a problem hiding this comment.
@thewatermethod What concerns do you have other then it has not been touched in 6 years?
There was a problem hiding this comment.
I was referring to this: wadackel/dot-wild#18
| * @param value2 - The second value to compare. | ||
| * @returns True if the values are deeply equal, false otherwise. | ||
| */ | ||
| const isDeepEqual = (value1: any, value2: any): boolean => { |
There was a problem hiding this comment.
lodash has this I think if you want to just use theirs
src/lib/modelUtils.ts
Outdated
| * @param value - The value to be checked. | ||
| * @returns A boolean indicating whether the value is an object or not. | ||
| */ | ||
| const isObject = (value: any): boolean => ( |
There was a problem hiding this comment.
I believe this is equivalent to "isPlainObject" from lodash if we don't want to maintain our own (we're already using lodash all over the place, so it wouldn't be adding a dependency)
|
|
||
| // Get the column information using the describe method | ||
| const getColumnInformation = async (model: typeof Model) => { | ||
| const tableDetails = await model.describe(); |
There was a problem hiding this comment.
do you think this needs a try/catch?
There was a problem hiding this comment.
Sure, what do you propose we do in the catch? Log, call the handleErrors, something else?
There was a problem hiding this comment.
Depends on whether you think it makes sense to return an empty array or blow up
| model: typeof Model, | ||
| type, | ||
| ) => { | ||
| const modelData = await getColumnInformation(model); |
There was a problem hiding this comment.
do you think this needs a try/catch?
There was a problem hiding this comment.
I won't iterate this question out for each of these functions, but same question for the rest of the stuff in this file, especially the ones that could have rejected promises in the middle, or ones with a bunch of second-level object property accessing.
Once this builds successfully, I'm also curious about the test coverage with all these nested if statements. It's possible some of this could be simplified if we find paths of code that are so unlikely as to not need tests.
There was a problem hiding this comment.
great thought. I don't know. I have tests for some if it. But I don't know how to see coverage when I am still failing lint as a whole.
|
Scanning ahead, I noticed a bunch of empty files - I'm assuming those aren't to be deleted (i.e. still a WIP)? |
| [DataTypes.JSON.key]: 'object', | ||
| [DataTypes.JSONB.key]: 'object', | ||
| [DataTypes.NOW.key]: undefined, | ||
| [DataTypes.BLOB.key]: undefined, |
There was a problem hiding this comment.
So these undefined mappings simply don't have a corresponding value?
There was a problem hiding this comment.
the list is all the types defined in the sequelize.DataTypes. but many are very rarely ever used in any context, and not in our context. The only one that is marked as unknow that we use on any table we use is DataType.ARRAY. To have a mapping for that I would also need to identify the type of the item in the array. So I think this is a not going to be as problem as all the tables in this PR don't use the array datatype.
| * @param {string} input - The PascalCase string to convert. | ||
| * @returns {string} - The converted camelCase string. | ||
| */ | ||
| const pascalToCamelCase = (input) => (input |
There was a problem hiding this comment.
_.camelCase(input)
|
|
||
| const findAll = async ( | ||
| data: { validFor: typeof ENTITY_TYPE[keyof typeof ENTITY_TYPE] }, | ||
| ) => genericEnum.findAll(Reasons, data); |
There was a problem hiding this comment.
many of these functions could be made generic, IE, we only need a separate findAll for reasons and status if they are doing something besides passing a model through.
There was a problem hiding this comment.
Also, in this case, I believe the tablename should be Reason (singular)
There was a problem hiding this comment.
I have a generic implementation, these functions call it. They pass into it the "unique" thing each needs. How would you change it?
There was a problem hiding this comment.
const findAll = async (
model: ModelType, data: { validFor: typeof ENTITY_TYPE[keyof typeof ENTITY_TYPE] },
) => genericEnum.findAll(model, data);etc
There was a problem hiding this comment.
discussed this on a call - clarified that what I'd really be after is no middleman function at all, IE we just call genericEnum.findAll where appropriate. Also stressed that it's a personal preference thing rather than a "make this change right now" kind of suggestion.
| * */ | ||
|
|
||
| //--------------------------------------------------------------------------------- | ||
| await queryInterface.createTable('Foiaable', { |
There was a problem hiding this comment.
Prob havent gotten there yet but how do we use Foiaable and validFor tables?
There was a problem hiding this comment.
@AdamAdHocTeam
Foiaable table identifies which columns for which tables are "read-only" after the isFoiaable flag is set. This allows some values to still be updated while maintaining the integrity of other columns.
ValidFor is used in conjunction with many of the enum-esk tables to allow similar enums to be located in the same table. Like "Statuses" hold the valid statuses for Training Events, Training Sessions, Goals, Objectives, and Collaborators. So they all reside in the same table and have the mapsTo construct to allow extensibility without each requiring their own table.
Future-future plan would be to have an admin page to manage these enum-esk values. Think Audience, national centers, participants, reasons, roles, statuses, support types, target populations, and topics. Let them manage the values. Then the UI and data just run off the admin maintained data. Again, future-future plan.
…ad-Start-TTADP into TTAHUB-955/generic-report
Description of change
How to test
Issue(s)
Checklists
Every PR
Production Deploy
After merge/deploy