Skip to content

Commit

Permalink
Fixed: Show "Not Available" instead of "?" when no InCinemas date
Browse files Browse the repository at this point in the history
  • Loading branch information
@Qstick
Qstick committed Feb 26, 2020
1 parent 3de65da commit 64e9e48
Showing 1 changed file with 1 addition and 12 deletions.
13 changes: 1 addition & 12 deletions frontend/src/Movie/MovieStatus.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ function MovieStatus(props) {

const hasMovieFile = !!movieFile;
const isQueued = !!queueItem;
const hasReleased = isAvailable;
const hasReleased = isAvailable && inCinemas;

if (isQueued) {
const {
Expand Down Expand Up @@ -74,17 +74,6 @@ function MovieStatus(props) {
);
}

if (!inCinemas) {
return (
<div className={styles.center}>
<Icon
name={icons.TBA}
title="TBA"
/>
</div>
);
}

if (!monitored) {
return (
<div className={styles.center}>
Expand Down

4 comments on commit 64e9e48

@geogolem
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I agree with the changes here - but I may be wrong.. so I just want to clarify.

I think the function that generates isAvailable already takes this into account.

What about cases where the inCinemas date is unknown but the physical release is known and past? isAvailable is true if the physical release date is known and past even if inCinemas date is unknown. With this change, lack of inCinemas date knowledge will mark movies as Unavailable even if they definitely released quite a while ago and this is known because of known physical release date.

I might be totally off base here, but I just wanted to clarify..

There is a difference between ? and "Not Available".. I think.

Please let me know what you think.

Having said that, I think the second part of this change, may be necessary. The reason I noticed this issue is because the changes conflicted with the branch I have which reverts the colour to green. I realize the reversion to the green colour is not totally agreed upon, but there were a couple other changes I had in that branch. Perhaps you could take a look:

https://github.com/geogolem/Radarr/tree/MovieStatusFix

maybe the other changes could be pushed, without reverting the colour...
Not sure if I am totally missing the point here, but thought I would point it out.

@Qstick
Copy link
Member Author

@Qstick Qstick commented on 64e9e48 Feb 27, 2020

Choose a reason for hiding this comment

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

Main thing I was after is getting rid of the icon, I see your point to the if physical but no in cinemas scenario. How about this, if no in cinemas AND no physical date then not available, else if we have a physical call it missing?

This is only the UI as well and has no effect on backend processing.

@geogolem
Copy link
Contributor

Choose a reason for hiding this comment

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

Main thing I was after is getting rid of the icon, I see your point to the if physical but no in cinemas scenario. How about this, if no in cinemas AND no physical date then not available, else if we have a physical call it missing?

This is only the UI as well and has no effect on backend processing.

I think the changes I made in my MovieStatusFix branch accomplish what you are proposing... but I need to double check, if I do, I will create another branch with just those changes (they really should be in their own branch anyway) and open a PR for you to take a look. I think the issue you were trying to fix regarding the icon etc., I fixed in my branch a while ago, but didnt bother creating a separate PR for it, because I wasn't really sure what people wanted. Anyway, once I do a little more investigation and open a PR, I will link to it here, or report back.

@geogolem
Copy link
Contributor

Choose a reason for hiding this comment

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

@Qstick : I created a new PR: #4201

this new PR basically just isolates some changes from this PR: #4012

which likely should have been the case from the beginning anyway.

I'm not 100% certain if my proposed changes do in fact resolve the issue you were aiming at with your changes, but I believe it does.

Please sign in to comment.