Add linkedEvent column to OrderPickupEvent Table #385

nik-dange · 2024-01-04T07:23:14Z

Description

Need a way to tie order pickup events to real events so that frontend can display things like the event graphic

Changes

Added a column to OrderPickupEvents table to link to Events
Added an accompanying relation

Type of Change

Patch (non-breaking change/bugfix)
Minor (non-breaking change which adds functionality)
Major (fix or feature that would cause existing functionality to not work as
expected)
Documentation (A change to a README/description)
Continuous Integration/DevOps Change (Related to deployment steps, continuous integration
workflows, linting, etc.)
Other: (Fill In)

If you've selected Patch, Minor, or Major as your change type, make sure to bump the version before merging in package.json!

Testing

I have tested that my changes fully resolve the linked issue ...

locally.
on the testing API/testing database.
with appropriate Postman routes. Screenshots are included below.

Checklist

I have performed a self-review of my own code.
I have followed the style guidelines of this project.
I have appropriately edited the API version in the package.json file.
My changes produce no new warnings.

Screenshots

Please include a screenshot of your Postman testing passing successfully.

github-actions · 2024-01-04T07:23:27Z

Thanks for contributing!
If you've made changes to the API's functionality, please make sure to bump the package
version—see this guide to semantic versioning for details—and
document those changes as appropriate.

nik-dange · 2024-01-04T07:23:39Z

I do api change tmrw I am sleepy

types/ApiRequests.ts

api/validators/MerchStoreRequests.ts

nik-dange · 2024-01-11T06:15:19Z

tests/data/MerchFactory.ts

@@ -110,6 +110,7 @@ export class MerchFactory {
      orderLimit: FactoryUtils.getRandomNumber(1, 5),
      status: OrderPickupEventStatus.ACTIVE,
      orders: [],
+      linkedEvent: null,


Should I import the EventFactory and generate an actual dummy event to put in here? Are there any other test cases that could be beneficial for this feature?

I don't actually think so. I actually don't think it is clean to attach a dummy event everytime we mock a pickupEvent as sometimes pickupEvent doesn't have an actual linkedEvent attached.

I don't believe it is necessary to implement a solution for this now, but for future reference, if there is a new feature that actually depends on linkedEvent, we should come up with a nice way (such as taking an optional boolean flag variable to attach event) to attach dummy linkedEvent to the mocked pickupEvent.

nik-dange · 2024-01-11T06:16:11Z

models/OrderPickupEventModel.ts

+  @OneToOne((type) => EventModel, { nullable: true })
+  @JoinColumn({ name: 'linkedEvent' })
+  linkedEvent: EventModel;
+


Can you double check this relation–I found a few different syntax examples online but I believe this establishes the correct one-way relation.

This looks good to me.

dowhep

A few nits. Remember to attach postman screenshots.

dowhep · 2024-01-12T09:57:51Z

services/MerchStoreService.ts

+
+      if (changes.linkedEventUuid) {
+        const linkedRegularEvent = await this.getLinkedRegularEvent(changes.linkedEventUuid);
+        updatedPickupEvent.linkedEvent = linkedRegularEvent;


u should save the event into the repository after editing the linkedEvent

lets also create a unit test to test for this

This method is called when a pickup event is edited, so only the UUID of the linkedEvent would be changed. I made a call to the upsert function on line 1076, which should be the only call necessary to save this change, right?

oh yeah you are right about that

dowhep · 2024-01-12T10:06:29Z

tests/data/MerchFactory.ts

@@ -110,6 +110,7 @@ export class MerchFactory {
      orderLimit: FactoryUtils.getRandomNumber(1, 5),
      status: OrderPickupEventStatus.ACTIVE,
      orders: [],
+      linkedEvent: null,


I don't actually think so. I actually don't think it is clean to attach a dummy event everytime we mock a pickupEvent as sometimes pickupEvent doesn't have an actual linkedEvent attached.

I don't believe it is necessary to implement a solution for this now, but for future reference, if there is a new feature that actually depends on linkedEvent, we should come up with a nice way (such as taking an optional boolean flag variable to attach event) to attach dummy linkedEvent to the mocked pickupEvent.

dowhep · 2024-01-12T10:08:59Z

models/OrderPickupEventModel.ts

+  @OneToOne((type) => EventModel, { nullable: true })
+  @JoinColumn({ name: 'linkedEvent' })
+  linkedEvent: EventModel;
+


This looks good to me.

nik-dange · 2024-01-15T23:28:55Z

dowhep

LGTM 🚀 🚀 🚀 🚀 🚀

dowhep · 2024-01-19T22:49:30Z

services/MerchStoreService.ts

+
+      if (changes.linkedEventUuid) {
+        const linkedRegularEvent = await this.getLinkedRegularEvent(changes.linkedEventUuid);
+        updatedPickupEvent.linkedEvent = linkedRegularEvent;


oh yeah you are right about that

…into feature/linked-event-column merging from master

nik-dange added 2 commits January 3, 2024 23:19

added migration and relation

961db7d

maybe a better relation idk

cb6864c

nik-dange added 2 commits January 4, 2024 23:05

added types throughout request/response

0bcfc38

nvm i think this is correct

71af1c9

nik-dange commented Jan 5, 2024

View reviewed changes

types/ApiRequests.ts Outdated Show resolved Hide resolved

nik-dange commented Jan 5, 2024

View reviewed changes

api/validators/MerchStoreRequests.ts Outdated Show resolved Hide resolved

nik-dange added 3 commits January 8, 2024 21:57

a lot of random changes

147da54

added OrderPickupEventModel relation

32daa74

linting

15e2d71

nik-dange commented Jan 11, 2024

View reviewed changes

nik-dange marked this pull request as ready for review January 11, 2024 06:19

nik-dange requested review from sumeet-bansal and shravanhariharan2 as code owners January 11, 2024 06:19

dowhep requested changes Jan 12, 2024

View reviewed changes

nik-dange added 2 commits January 19, 2024 17:13

unit test

c6db3f5

linting + unit test

175af99

dowhep approved these changes Jan 20, 2024

View reviewed changes

nik-dange added 3 commits January 20, 2024 13:20

added faker internet urls to eventfactory

7b3a257

Merge branch 'master' of https://github.com/acmucsd/membership-portal …

c602d86

…into feature/linked-event-column merging from master

bumped package version

09c1a0b

nik-dange merged commit 65cd022 into master Jan 20, 2024
5 checks passed

nik-dange deleted the feature/linked-event-column branch February 29, 2024 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add linkedEvent column to OrderPickupEvent Table #385

Add linkedEvent column to OrderPickupEvent Table #385

nik-dange commented Jan 4, 2024

github-actions bot commented Jan 4, 2024

nik-dange commented Jan 4, 2024

nik-dange Jan 11, 2024

dowhep Jan 12, 2024

nik-dange Jan 11, 2024

dowhep Jan 12, 2024

dowhep left a comment

dowhep Jan 12, 2024

dowhep Jan 12, 2024

nik-dange Jan 15, 2024

dowhep Jan 19, 2024

dowhep Jan 12, 2024

dowhep Jan 12, 2024

nik-dange commented Jan 15, 2024

dowhep left a comment

dowhep Jan 19, 2024

Add linkedEvent column to OrderPickupEvent Table #385

Add linkedEvent column to OrderPickupEvent Table #385

Conversation

nik-dange commented Jan 4, 2024

Description

Changes

Type of Change

Testing

Checklist

Screenshots

github-actions bot commented Jan 4, 2024

nik-dange commented Jan 4, 2024

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

dowhep 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

nik-dange commented Jan 15, 2024

dowhep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment