Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix(Sales): Resolve "characters not found" issue #462

Closed
wants to merge 6 commits into from

Conversation

SpeedyD
Copy link
Contributor

@SpeedyD SpeedyD commented Dec 9, 2022

Referring to Issue #459 that is-

I feel like this is a form of band-aid solution, but it should look and work decently. Please look it over thoroughly.

@itinerare itinerare self-assigned this Dec 9, 2022
Copy link
Contributor

@AW0005 AW0005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a little nitpick comment and thought the rest of this looked good!

But I also noticed in the description you felt like this was a band-aid solution so I did a little poking around and I think another option might be to update the relationship on Sales for the SalesCharacter to be this: $this->hasMany('App\Models\Sales\SalesCharacter', 'sales_id')->has('character')`

So it would never return SalesCharacters as a part of the collection at all, if their character is undefined. The downside to that though, is then there's no messaging to the user that a character was deleted and that's why it's missing. 🤔 Not sure which would be more preferable.

@itinerare
Copy link
Collaborator

Yeah I had the idea to have better checking on the interface itself so that the information entered pertinent to the sale was still accessible; ideally I think this would be the case for both the admin and public-facing interfaces so that records are maintained, just replacing relevant bits with [deleted character] or somesuch as is done for e.g. logs.

@SpeedyD
Copy link
Contributor Author

SpeedyD commented Mar 11, 2023

@AW0005, @itinerare had personally told me they'd look things over and work on it, so I'm leaving it 100% up to them from here.
I just wanted to set up a temporary solution that worked.. and it does, even if it's not that clean. ^^;

@itinerare
Copy link
Collaborator

itinerare commented Mar 11, 2023

Yeah I had some bold thoughts about getting character info by virtue of it being soft-deleted rather than deleted-deleted... though tbh this probably wouldn't work as well as I'd like considering that iirc the core character is the only thing to which that applies and not e.g. the info on the image object, traits, etc, which are kinda the relevant bits of info that would contextualize the sale info. At this rate probably what I would end up doing is just cleaning up the display in line w my comment above.

@itinerare itinerare removed their assignment May 28, 2023
@SpeedyD
Copy link
Contributor Author

SpeedyD commented Jun 12, 2023

Just refreshing the branch by putting all the new stuff in..

@SpeedyD
Copy link
Contributor Author

SpeedyD commented Jun 12, 2023

..Hrm, I have an idea how to do this the way Merc maybe wants, but I think I need #615 resolved first. It might help the process along..

So I'm converting this to draft for now.

@SpeedyD SpeedyD marked this pull request as draft June 12, 2023 19:15
…fix/missing-character-sale

aka feat(sales): use character image at the time a character is added to a sale (corowne#615)
@SpeedyD SpeedyD changed the title This will resolve characters not found on sales pages. Fix(Sales): Resolve "characters not found" issue Aug 21, 2023
@SpeedyD
Copy link
Contributor Author

SpeedyD commented Dec 14, 2023

Don't know if still needed, but I've opted to at least push the dev branch in for now, resolving the one conflict

@SpeedyD
Copy link
Contributor Author

SpeedyD commented Dec 14, 2023

..Scratch that. Looking at the changes that have been done and the two edits in here do not even get reached anymore. Closing PR.

@SpeedyD SpeedyD closed this Dec 14, 2023
@SpeedyD SpeedyD deleted the fix/missing-character-sale branch December 14, 2023 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants