Implement initial bring forward features #49

wilwong89 · 2024-03-28T21:20:24Z

Description

Implement the initial bring forwards feature with the following changes:
-frontend to set bring forward status and dates, and display that information
-db model changes to store bring forward status and dates
-update type definition and unit tests

Showcase-3432

Types of changes

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the CONTRIBUTING doc
I have checked that unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

Note that a db migration is necessary and the migration file is included.

github-actions · 2024-03-28T21:21:02Z

Coverage Report (Frontend)

Totals
Statements:	24.38% ( 425 / 1743 )
Methods:	24.68% ( 78 / 316 )
Lines:	28.03% ( 275 / 981 )
Branches:	16.14% ( 72 / 446 )

github-actions · 2024-03-28T21:29:37Z

Coverage Report (Application)

Totals
Statements:	45.3% ( 405 / 894 )
Methods:	35.91% ( 65 / 181 )
Lines:	61.1% ( 289 / 473 )
Branches:	21.25% ( 51 / 240 )

codeclimate · 2024-03-28T21:30:08Z

Code Climate has analyzed commit 1036aa8 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 50.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 38.7% (2.3% change).

View more on Code Climate.

kyle1morel · 2024-03-28T22:43:31Z

app/src/db/migrations/20240327213908_bring-forward-feat.ts

@@ -0,0 +1,31 @@
+import type { Knex } from 'knex';


Let's rename this to file to 20240327000000_001-bring-forward.ts. I should've followed our historical naming convention with the init migration, but it slipped by before going to prod. So let's start this now.

kyle1morel · 2024-03-28T22:43:35Z

app/src/db/migrations/20240327213908_bring-forward-feat.ts

+    .then(() =>
+      knex.schema.alterTable('note', function (table) {
+        table.timestamp('bring_forward_date', { useTz: true });
+        table.text('bring_forward_state').defaultTo('');


Can we make this nullable instead of defaulting to an empty string. NULL is a better representation of no value.

kyle1morel · 2024-03-28T22:43:37Z

app/src/db/models/note.ts

@@ -30,6 +32,8 @@ export default {
    return {
      noteId: input.note_id,
      activityId: input.activity_id,
+      bringForwardDate: input.bring_forward_date?.toISOString() ?? null,
+      bringForwardState: input.bring_forward_state || '',


Can we make this nullable instead of defaulting to an empty string. NULL is a better representation of no value.

kyle1morel · 2024-03-28T22:43:41Z

app/src/validators/note.ts

@@ -8,6 +8,8 @@ const schema = {
    body: Joi.object({
      createdAt: Joi.date().required(),
      activityId: activityId,
+      bringForwardDate: Joi.date().iso().allow(null),
+      bringForwardState: Joi.string().allow(null),


Consider: If we are allowing null and expecting a value otherwise, this can be min(1) to disallow empty set.

kyle1morel · 2024-03-28T22:43:46Z

frontend/src/types/Note.ts

+  bringForwardDate?: string | null;
+  bringForwardState?: string | null;


Should not have | null here as they are marked as optional.

kyle1morel · 2024-03-28T22:58:01Z

frontend/src/components/note/NoteModal.vue

+    .nullable()
+    .when('noteType', {
+      is: (noteType: string) => noteType === NOTE_TYPES.BRING_FORWARD,
+      then: (schema) => schema.test('must select date', 'A date must be selected', (value) => value instanceof Date),


Let's make the error message match the required error: Bring forward date is a required field

TimCsaky

looking good!

TimCsaky · 2024-04-03T00:05:30Z

frontend/src/components/note/NoteModal.vue

@@ -61,6 +95,8 @@ watch(visible, (newValue) => {
  nextTick().then(() => {
    if (newValue && formRef.value) {
      formRef.value.setFieldValue('createdAt', new Date());


if the 'Date' field is read-only, and we always want it to be the current date/time i question whether it needs to be on the form at all. something i could ask Tyler about i guess

TimCsaky · 2024-04-03T00:12:29Z

frontend/src/components/note/NoteCard.vue

      <Divider type="solid" />
    </template>
    <template #content>
      <div class="grid nested-grid">
        <!-- Left column -->
-        <div class="col-12 md:col-6 lg:col-4">


these columns are too wide on big screens. lg:col-2 is probably enough

TimCsaky · 2024-04-03T16:28:55Z

app/src/db/migrations/20240327000000_001-bring-forward.ts

+    .then(() =>
+      knex.schema.alterTable('note', function (table) {
+        table.timestamp('bring_forward_date', { useTz: true });
+        table.text('bring_forward_state').nullable();


fyi. you don't need to add nullable(). fields are nullable by default on column creation

wilwong89 added 2 commits March 26, 2024 17:49

Bring forward frontend features

5d46c03

Note model and functionality changes

794c823

wilwong89 requested review from norrisng-bc, TimCsaky, jatindersingh93 and kyle1morel as code owners March 28, 2024 21:20

wilwong89 force-pushed the feature/bring-forward branch from a0ae71e to a82d0a3 Compare March 28, 2024 21:28

kyle1morel reviewed Mar 28, 2024

View reviewed changes

wilwong89 force-pushed the feature/bring-forward branch from a82d0a3 to b37de12 Compare March 28, 2024 23:33

Bring forward feature migration, unit tests, and validation

1036aa8

wilwong89 force-pushed the feature/bring-forward branch from b37de12 to 1036aa8 Compare March 28, 2024 23:43

kyle1morel approved these changes Mar 28, 2024

View reviewed changes

wilwong89 closed this Mar 29, 2024

wilwong89 reopened this Mar 29, 2024

norrisng-bc approved these changes Apr 2, 2024

View reviewed changes

TimCsaky reviewed Apr 3, 2024

View reviewed changes

kyle1morel changed the base branch from master to feature/bring-forward-date April 3, 2024 20:43

kyle1morel merged commit 8230085 into feature/bring-forward-date Apr 3, 2024
32 of 33 checks passed

kyle1morel deleted the feature/bring-forward branch April 3, 2024 20:44

kyle1morel mentioned this pull request Apr 5, 2024

Implement bring forward functionality #59

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement initial bring forward features #49

Implement initial bring forward features #49

wilwong89 commented Mar 28, 2024

github-actions bot commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

codeclimate bot commented Mar 28, 2024 •

edited

Loading

kyle1morel Mar 28, 2024

kyle1morel Mar 28, 2024

kyle1morel Mar 28, 2024

kyle1morel Mar 28, 2024

kyle1morel Mar 28, 2024

kyle1morel Mar 28, 2024

TimCsaky left a comment

TimCsaky Apr 3, 2024

TimCsaky Apr 3, 2024

TimCsaky Apr 3, 2024

		bringForwardDate?: string \| null;
		bringForwardState?: string \| null;

Implement initial bring forward features #49

Implement initial bring forward features #49

Conversation

wilwong89 commented Mar 28, 2024

Description

Types of changes

Checklist

Further comments

github-actions bot commented Mar 28, 2024 • edited Loading

Coverage Report (Frontend)

github-actions bot commented Mar 28, 2024 • edited Loading

Coverage Report (Application)

codeclimate bot commented Mar 28, 2024 • edited Loading

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

TimCsaky 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

github-actions bot commented Mar 28, 2024 •

edited

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

codeclimate bot commented Mar 28, 2024 •

edited

Loading