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: make CFS optional like adding org info wizard step 1 #5302

Merged
merged 2 commits into from
Oct 15, 2020
Merged

fix: make CFS optional like adding org info wizard step 1 #5302

merged 2 commits into from
Oct 15, 2020

Conversation

maze-runnar
Copy link
Contributor

Fixes #5300

Checklist

  • I have read the Contribution & Best practices Guide.
  • My branch is up-to-date with the Upstream development branch.
  • The acceptance, integration, unit tests and linter pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@auto-label auto-label bot added the fix label Oct 13, 2020
@vercel
Copy link

vercel bot commented Oct 13, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/eventyay/open-event-frontend/omthgpq46
✅ Preview: https://open-event-frontend-git-wizard-grey.eventyay.vercel.app

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #5302 into development will decrease coverage by 0.10%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #5302      +/-   ##
===============================================
- Coverage        22.80%   22.69%   -0.11%     
===============================================
  Files              491      491              
  Lines             5241     5243       +2     
  Branches            37       37              
===============================================
- Hits              1195     1190       -5     
- Misses            4041     4048       +7     
  Partials             5        5              
Impacted Files Coverage Δ
.../components/forms/wizard/sessions-speakers-step.js 0.00% <0.00%> (ø)
app/models/event.js 31.25% <0.00%> (-18.75%) ⬇️
app/controllers/index.js 27.27% <0.00%> (-9.10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5cae450...23ba7c6. Read the comment docs.

@mariobehling
Copy link
Member

Was not able to fully test due to Stripe saving issue. Updated branch to see if it works then.

Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Ok, in regards to the wizard it is working fine. However the CfS is still visible on the public page:

https://open-event-frontend-git-wizard-grey.eventyay.vercel.app/e/5ac8e4d9/cfs

It shows "closed" instead:

  • The page should not be available if the CfS is deactivated.

Screenshot from 2020-10-14 10-54-36

@iamareebjamal
Copy link
Member

iamareebjamal commented Oct 14, 2020

@maze-runnar Please delete CfS if the organizer disables CfS

@maze-runnar
Copy link
Contributor Author

maze-runnar commented Oct 14, 2020

resetCFS() {
      this.data.speakersCall.deleteRecord();
 },

after applying this, when I am again trying to change it -
: Attempted to set 'startsAt' to '1602618900000' on the deleted record <speakers-call:8>

And then after saving and again opening
TypeError: Cannot read property 'announcement' of null

this is working fine -

resetCFS() {
      this.set('data.speakersCall.announcement', '');
    },

@mariobehling
Copy link
Member

@maze-runnar Please delete CfS if the organizer disables CfS

Does this mean the data will be deleted? This would be different to other fields where the data is still saved even if the field gets deactivated.

@iamareebjamal
Copy link
Member

iamareebjamal commented Oct 14, 2020

Not deleting the announcement means that we'll have to add another field in DB and API and also add validation that enabling CFS actually contains CFS and disabling CFS actually has no CFS.

Note that only announcement gets deleted, not other CFS fields, as we don't show CFS without announcements

@mariobehling
Copy link
Member

Not deleting the announcement means that we'll have to add another field in DB and API and also add validation that enabling CFS actually contains CFS and disabling CFS actually has no CFS.

It seems deleting the CfS is the quick solution, ok. But could you add an issue to follow up here, cause it would be better if the announcement would not be deleted when it gets deactivated. Reason is: Organizers play around with features. Also they could click a field by accident. There are many reasons organizers would not want data (like the announcement) to be deleted. So, which deleting the announcement solves the current issue quickly it should not be a final fix.

As a matter of fact if we delete the data we should actually have a pop up "You are deleting the announcement. Yes/No". This is all too much to go through. From the user perspective the best would be if the data does not get deleted simply.

So, I suggest:

  1. Delete it for now.
  2. Open an issue and change the functionality in a way that no data gets deleted when a field gets deactivated (just like most of the other fields).

Does this work?

@iamareebjamal
Copy link
Member

I have created the issue and we'll do that in future, but the problem I was talking about, desynchronization of the field and data already exists in the system for most fields. That's why there were so many errors in the past about:

  • Stripe checkbox and stripe connection checked and not connected
  • Sessions present but not showing on public
  • Sessions not present but showing on public
  • Schedule present but not showing on public
  • Schedule not present but showing on public
  • Speakers present but not showing on public
  • Speakers not present but showing on public

So, adding a switch which is independent of data always creates edge cases which may eventually be fixed after many iterations, but are fragile. For example, above issues are now fixed on frontend, but there is no check on the backend, so it is entirely possible via API to say the event accepts stripe for payment but not add the connection. And adding such checks in server is very difficult because the change in stripe auth and connection may happen before or after each other and thus create race problems.

So, we'll add that feature in near future but it comes at development and bug cost.

@iamareebjamal iamareebjamal merged commit 73d54dd into fossasia:development Oct 15, 2020
@maze-runnar maze-runnar deleted the wizard-grey branch October 15, 2020 07:44
@mariobehling
Copy link
Member

I understand. I wonder how others are doing it. An alternative would be to add a message "do you really want to do this?" every time a user switches off a feature. This was how Windows worked years ago and users hated it. So, it seems to me we need to go the way as described by you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wizard Step 5: Make Call for Speakers Optional
3 participants