codefordenver / Comrad

Open-source web application for radio stations to manage show schedules, traffic and compliance
ISC License
25 stars 9 forks source link

Custom record_audio value should always be present in show JSON #877

Open seankwilliams opened 1 year ago

seankwilliams commented 1 year ago

There is a custom.record_audio value for shows. Sometimes, this isn't in the data at all. It should always be present, and should default to false.

George from KGNU says: ahh - fyi - it looks like TUCRadio was missing the value - last Sunday (11/5/23) at noon, it alternates weeks with Tributaries.

Also missing this is: RMCRRegionalRoundup (Mondays)

kmid5280 commented 8 months ago

@seankwilliams If I'm understanding correctly that this is a missing attribute on the back end, my initial thoughts here are to change two files. In server\v1\controllers\events\createInstance.js and server\v1\controllers\events\update.js, add a conditional that checks for a value of custom.record_audio and set it to false if it doesn't exist. Then we can bulk update the library objects after the change is made. Does this sound like it's on the right track?

seankwilliams commented 8 months ago

@kmid5280 Please feel free to give that a try and test things out! I am not sure the exact conditions that end up with record_audio having no value at all, so we'll have to do some testing around it, but the places you mentioned sound like a good place to start.

kmid5280 commented 8 months ago

@seankwilliams This looks like a two-part issue, one for posting and one for updating existing shows with a new custom.record_audio value. Let's start with posting a new show. I'm finding that when I create a new show on the Show Calendar, it doesn't set a value for show_details.custom.record_audio unless I check the Record Audio box on the modal. It looks like we want it to set a default value to false when posting.

This is a custom value, which appears to be imported from server/v1/config/base.js. To fix this, I need to find a way to set a default value for custom.record_audio when a new show is posted. We could try to set an initial value in client/src/components/Shows/NewShow/Form.js, though I'm concerned that if we only set the default value on the front end on the form, then it won't set that value if the show is posted some other way.

We could possibly add a conditional to the postShow method in client/src/redux/show/showActions.js to check for the custom.record_audio value and set it to false if it doesn't exist.

Wondering if we should set a default value for the checkbox in server/v1/config/base.js? Do you know the correct attribute for that? I tried adding defaultValue: true and defaultChecked: true on line 72 to no effect.

I'm having some trouble doing this with custom fields. Since by default, it is not creating a custom attribute at all if the Record Audio box is unchecked, I'm not sure how to get it to check for the custom.record_audio attribute when the show is posted and returns an error when I try to console.log it. This happens for example in client/src/redux/show/showActions.js. Do you have any suggestions here?

kmid5280 commented 8 months ago

@seankwilliams Also, I notice in some cases that when set, the custom.record_audio value is set to 1 instead of true. Does that matter? Do you suppose we'll have to update any of these to true at any point?

seankwilliams commented 8 months ago

@kmid5280 For purposes of this, let's not worry about ones that are set to 1 instead of true. Ideally they'd all be consistent (so if you see a way to do that, go for it!), but for the scope of this issue, the main thing to address is when the custom.record_audio value is not set because that's breaking a script that KGNU has set up (outside of Comrad)

I think your testing is on the right track but I do not have any suggestions off the top of my head for how to address. We'd have to review (1) the implementation of custom attributes in the code base (this is specific to Comrad), and (2) Redux Form and how it handles checkboxes that are not selected. It's also possible that the solution could be implemented on the back-end if necessary when creating & updating a show.

kmid5280 commented 8 months ago

@seankwilliams I'm learning that Mongoose will not automatically set a value to an attribute listed in the schema if the attribute is empty. Meaning, it will not create a custom attribute at all if there is nothing inside of it. There is a way to override this behavior and have it list all blank fields as empty objects instead of omitting them, though I'm not sure that's the kind of behavior we want as this could apply to all possible fields: https://mongoosejs.com/docs/guide.html#minimize

My thought is, as a workaround, we could add the following conditionals in client/src/redux/show/showActions.js starting on line 43. This first checks if there is a custom attribute, sets it to an empty object if not, then checks if there is a custom.record_audio attribute, which it will set to false if not. After checking, this does seem to ensure that there will be a default value of false for this field if not specified. (Granted, this is only addressing the issue concerning posting new shows, we'll have to deal with the issue of updating existing shows to contain this attribute separately.)

    if (show.show_details.custom == null) {
      show.show_details.custom = {};
    }

    if (show.show_details.custom.record_audio == null) {
      show.show_details.custom.record_audio = false;
    }
seankwilliams commented 8 months ago

@kmid5280 Good research, and I think this fix makes a lot of sense!

In a perfect world, everything for the custom fields would be totally powered by the settings (server\v1\config\base.js) rather than hardcoded. There are some API calls that pull down the custom field settings to the front-end into Redux, where they are handled there. So ideally, we'd somehow find out that record_audio needs to be set to false from those settings rather than putting the property name record_audio directly in the code. This makes it so other stations than KGNU can adopt Comrad and customize it to their settings. Maybe the other station has no use for record_audio, but does want a different custom attribute that should always be true or false.

However, let's not let the perfect be the enemy of the good here, because your solution will work, and it solves a problem at KGNU where one of their scripts is crashing when this attribute is not present. Let's implement what you have! I can run a custom Mongo query to set this to false where it isn't arleady set in the database.

kmid5280 commented 8 months ago

@seankwilliams Thanks. Just to clarify, will this solution by itself be enough to tackle updating the existing entries as well?

seankwilliams commented 8 months ago

@kmid5280 I believe that will only affect the existing entries if the show is saved after your changes are in place.

kmid5280 commented 8 months ago

@seankwilliams Right now the changes are implemented in the postShow method, so it will set a record_audio value for new shows posted. So far there isn't anything that specifically targets updating an existing show. Do you want me to implement that too, or do you already have a way to take care of that? You mentioned being able to use a custom Mongo query and wasn't sure if it was necessary.

seankwilliams commented 7 months ago

@kmid5280 No need to implement something for existing shows! I can run a Mongo query for that. There really aren't many master show records, so even if it's hard to write a bulk query to do that, I could even update them one by one in 10 or 15 minutes.

seankwilliams commented 7 months ago

I am guessing I can write a bulk query for it no problem -- but I'm not as experienced querying Mongo as I am with SQL, so sometimes I get tripped up on it.