Remove feedbackcount col #1428
Remove feedbackcount col #1428
Conversation
…eation & destroying
Veikkosuhonen
force-pushed
the
remove-feedbackcount-col
branch
from
f33044f to
0a7f368
Compare
…ulation migration
| @@ -250,6 +250,7 @@ CourseUnit.hasMany(Summary, { sourceKey: 'groupId', foreignKey: 'entityId', as: | |||
|
|
|||
| Summary.belongsTo(FeedbackTarget, { foreignKey: 'feedbackTargetId', as: 'feedbackTarget' }) | |||
| FeedbackTarget.hasOne(Summary, { foreignKey: 'feedbackTargetId', as: 'summary' }) | |||
| FeedbackTarget.hasMany(Summary, { foreignKey: 'feedbackTargetId', as: 'summaries' }) | |||
There was a problem hiding this comment.
hasOne ja hasMany molemmat
There was a problem hiding this comment.
Joo, tuntuu typerältä mutta ajattelin näin kuitenkin. Yleensä feedbackTargettiin halutaan yksi 'summary' mukaan, ja nyt niiden summaryiden datat on samat joten ei väliä, mikä summary tulee mukaan.
Jossain kohti saatetaan kuitenkin haluta kaikki 'summaries', joka on tietokannan kannalta se oikea relaatio.
Eli nyt tuo alias 'summary' vain tekee kyselyn mukavemmaksi, kun ei tarvitse hakea listaa 'summaries' ja valita listalta yhtä.
src/server/models/summary.ts
Outdated
| @@ -63,6 +63,7 @@ Summary.init( | |||
| feedbackTargetId: { | |||
| type: INTEGER, | |||
| allowNull: true, | |||
| unique: true, | |||
There was a problem hiding this comment.
Hyvä huomio, poistan
| @@ -114,7 +119,7 @@ const getInterimFeedbackTargets = async (parentId, user) => { | |||
| as: 'courseRealisation', | |||
| required: true, | |||
| }, | |||
| { | |||
| /*{ | |||
There was a problem hiding this comment.
Kommentti miks tää kommentoitu, varmistus ettei jossain oo viitattu näihin?
There was a problem hiding this comment.
Joo, varmistin ettei käytetä missään.
Poistan kommentin.
| @@ -100,7 +100,7 @@ const buildSummariesForPeriod = async ({ | |||
| // Get all the feedback data and associated entities for this period. Then the rest is done JS side. | |||
| const feedbackTargets = await FeedbackTarget.findAll({ | |||
| where: { | |||
| userCreated: false, // Custom feedbacks may cause issues and dont contribute to stats anyways. | |||
| // userCreated: false, // Custom feedbacks may cause issues and dont contribute to stats anyways. | |||
There was a problem hiding this comment.
Miksi kommenteissa. Jotta välipalautekohteet tulee summariesiin? Nuo loopataan nyt kaikissa tän tiedoston yhteenvetolaskuissa, pitäs siellä ehkä suodattaa pois. Vaikkei varmaan vaikutusta kun ei yhteenvetokysymyksiä, mutta turhaa.
There was a problem hiding this comment.
Pitää varmistaa että välipalautteille luodaan summaryt, koska niistä saadaan feedbackCount tieto niille. Croni poistaa kaikki summaryt joten sen myös pitää luoda ne.
Mutta joo pitäisi ehkä filtteröidä niin että välipalautteiden statsit ei mene "ylöspäin" CU ja organisaatiostatseihin.
| feedbackTargets: _.uniqBy( | ||
| feedbackTargets.filter(fbt => !fbt.userCreated), | ||
| 'feedbackTargetId' | ||
| ), |
There was a problem hiding this comment.
Tää pitäs varmaan lisätä useampaan kohtaan (jos etsii kohdat missä 'feedbackTargets: _.uniqBy(feedbackTargets, 'feedbackTargetId')' eli courseRealisationSummaries ja courseUnitSummaries ja courseUnitGroupSummaries
Pitäskö tehdä lista 'basicFeedbackTargets' tms missä vaan noi userCreated false ja antaa se noille ?
Ja mitenhän toi feedbackTargetsSummaries jota käytetään sit muiden summaryiden laskennassa. pitäskö siitäkin olla basic versio, jota käytetään muissa paitsi sit ihan lopun ft-summary tallennuksessa, eli noita välipalautteita ei tarvittas missään muualla?
There was a problem hiding this comment.
Päivitin sen tälleen että muiden summaryiden laskennassa käytetään basicFeedbackTargetSummaries joista suodatettu userCreated
…n building summaries
Implements #1227
Fairly big fix and refactor so the
feedbackCountof an FBT is now stored in one place, thedataJSONB column of the corresponding Summary.There are no visible changes other than an elusive bug getting fixed.
Database changes
feedback_countcolumn of tablefeedback_targetsis droppedBugs fixed
The feedback count of a course had different values in some places, since some views used
summary.data.feedbackCountand some used the droppedfeedbackTarget.feedbackCountTesting
Automatic tests were updated to reflect the fix. A new test was added for a common bug case where the feedbackCounts would differ.
Implementation details...
The corresponding summaries of a course are now always updated when feedback is created or deleted, so they stay up to date. Cached values are also updated.
userCreated: truefeedbackTargets now have summaries created for them, when they are created, and when the summary cron runs.New files were written in typescript and some existing files were converted.
package-lock.json has a lot of changes (so the insertions - additions looks scary) - I will try to get rid of that.