Rename/refactor in models for clarity #533
Conversation
This was previously part of the accounts management, but that has since been removed.
Not prefixing with self makes subcategory a local variable, meaning that the method has no effect
Mostly fix name pluralization and slight logic tweaks
This is not a very necessary refactor, it was fine as is. This is mostly done to mirror the same pattern in another model. Removing the nil checks will not give errors when sorting valid objects. If it is invalid or being constructed, that may be an issue.
There was a problem hiding this comment.
Thank you very much for this. For the past while we have implement individual features or fixes and not gone back and reworked too much. It is very nice to see a bunch of changes in one go.
In general these are fantastic! Outside of the renaming of methods, this is a lot of very critical underlying changes. Some of these need very rigorous testing before they are deployed.
I know it is tedious, but I think it would be a very good idea to put all of the renames in one pull (can deploy quickly), and then separate out the ones that change/improve/streamline core logic into separate pulls. That way we can deploy them one at a time so that we can isolate potential issues one at a time and don't potentially break multiple parts of Tracker at once. If we see a full event process (creation/request, applications, dates, billing, invoicing) go through successfully then we can be reasonably certain a certain fix is good.
| # In the event list, we want to be able | ||
| # to share one runcrew list with multiple | ||
| # adjacent eventdate rows if they are identical | ||
| eventdates.chunk(&:full_roles).map { |roles, eds| eds } |
There was a problem hiding this comment.
Have you tested this? I am uncertain if chunk can accept a proc like that. I know that map can. My own opinion is that the ampersand shorthand notation is a little bit less clear.
There was a problem hiding this comment.
This does still work. I'm not particularly attached to either way of writing it, but I thought all the line breaking made it hard to read. Maybe it would be best to write the chunk with a one-line block? Up to you.
app/models/eventdate.rb
Outdated
| self.calltype == "literal" or self.calltype == "startdate" | ||
| end | ||
|
|
||
| def has_strike? | ||
| # Yes: literal, startdate |
There was a problem hiding this comment.
Small typo, should be enddate
There was a problem hiding this comment.
This is definitely a dangerous fix without a lot of testing. If a long time has gone without a validation being enforced, then adding it in after model entries have been created and modified can break it. Have you tested this thoroughly? Is it currently possible to create an entry that fails this validation?
There was a problem hiding this comment.
This commit is confusing and doesn't make clear the actual change made. I've added a commit which makes it more clear.
To summarize, self is needed on line 24 or the assignment will only be local. This is not used as a validation step; rather, it runs before validation. This change does not affect what entries are considered valid.
There was a problem hiding this comment.
Do not edit old migrations. You should create a new migration that renames the table/model. Performing this new rename migration before you commit will also update the schema.
There was a problem hiding this comment.
Migration changes will be reverted.
The accounts table itself never got deleted/had its entries removed, just no longer is used. If this information is used anymore, a rename in the database would be unclear, and removing columns would remove the data. Perhaps it would be better to leave it be or remove it, and add a new helper class which does the academic year?
There was a problem hiding this comment.
(see first migration file comment)
There was a problem hiding this comment.
(see first migration file comment)
app/models/current_academic_year.rb
Outdated
There was a problem hiding this comment.
I know that we don't run the tests, but there are some files in test/ that also need a rename (for consistency and to prevent later confusion).
Even if objects are invalid (e.g. if an InvoiceLine has an invalid category), allow them to still be sorted without raising errors.
Self is needed in the null_subcategory helper to prevent it creating a local variable. It is not needed elsewhere. This is not a validation step; rather, it runs before validation. Either of blank or nil continues to be allowed.
There are a number of parts of tracker that are ambiguous, retain irrelevant names, or are written in difficult-to-parse ways. For example, the
Account.magic_datemethod, which is actually the start of the current academic year as a string. This PR aims to make minimal changes to restore some clarity, intending to avoid opinionated rewrites or structural changes.Each commit is a change to a separate model file, in order to isolate the effects that the change has on other files.