-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add event cancellation #4273
base: develop
Are you sure you want to change the base?
Add event cancellation #4273
Conversation
… events during retrieval
Let me know if you need anything for this. |
…ncel-events # Conflicts: # app/backend/src/couchers/servicers/events.py
...kend/src/couchers/migrations/versions/46e6b6a3d9ed_added_is_cancelled_and_is_deleted_for_.py
Outdated
Show resolved
Hide resolved
@aapeliv @jesseallhands How do we want to display cancelled events? In my idea, they should later not show up in event searches at all, but should still show up under a tab "My Events" or so, if you joined one previously. In the current version, as we don't have too many events yet, I guess we display cancelled events as well, but should make it visually clear that they are/were cancelled. How should we do that best? (I am slightly limited by my frontend dev skills here) My idea for now was to grey them out a bit and include a "cancelled" badge. |
Yes, I think cancelled events should be under a separate cancelled events tab and the event itself should use a bright "cancelled!" pill (badge) to mark it, so if someone has the link to the event and they go to it, they easily see it has been cancelled. As far as deleted events go, those shouldn't be shown on the website at all, but should still be queryable on the backend. |
So I chose to include a switch for now to display cancelled events. I think soon, as discussed, we could have a new events page layout (see #3989). Cancelled events are greyed out and highlighted with a "cancelled" chip. On the page of a cancelled event the buttons are disabled and greyed out. Deleted events can't be queried at all through the API, but are still available in the database. I thought it might be better to avoid accidentally displaying a deleted event. But an alternative could be that only superusers can query deleted events. |
…k events and adjust tests
# Conflicts: # app/backend/src/couchers/errors.py # app/backend/src/couchers/servicers/admin.py # app/backend/src/couchers/servicers/events.py # app/backend/src/tests/test_admin.py # app/proto/admin.proto # app/proto/events.proto
There are 4 different places where events are currently listed, this is how I chose to deal with cancelled events for each of these:
I think viewing cancelled events does only make sense if wondering why an event I joined previously was cancelled or to look for other attendees/information and is therefore quite conscious. |
I think this is great! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks mostly good on the backend, a few small comments. I will have another look when I'm back on my computer!
|
||
|
||
def upgrade(): | ||
with op.get_context().autocommit_block(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this one do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Altering a type has to be run outside of a transaction, which this context manager allows, if I understood that correctly. Read this in Stackoverflow and on alembic docs.
Howevery, I saw that you did it in a previous migration without it, so maybe it is not necessary...
|
||
|
||
def downgrade(): | ||
raise Exception("Can't downgrade") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please combine the two migrations into one file. And please rebase on develop (you might need to change the down revision to the latest value) to keep a linear alembic history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I combined the migrations now - I merged develop into this branch already and added these migrations on top, so the history is linear.
closes #4178
Backend checklist
autoflake --exclude src/proto -r -i --remove-all-unused-imports src && isort . && black .
inapp/backend
develop
if necessary for linear migration historyWeb frontend checklist
make format
make lint