[$1000] Date of birth year deep link does not change the year #21261
Comments
kbecciv
added
Daily
|
Triggered auto assignment to @joekaufmanexpensify ( |
Bug0 Triage Checklist (Main S/O)
|
|
I can reproduce this. Though, I think it's odd that we change the link from IMO, the ideal behavior here is to stop changing the link when you adjust year of birth before saving. And then make it so you can't deep link to change your year of birth at all. This isn't something an actual user will frequently/ever be doing. |
joekaufmanexpensify
added
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~0190aa89bc7487f806 |
melvin-bot
bot
added
the
Help Wanted
|
Current assignee @joekaufmanexpensify is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @allroundexperts ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.Date of birth year deep link does not change the year What is the root cause of that problem?
App/src/components/CalendarPicker/index.js Lines 20 to 46 in 98bb603 What changes do you think we should make in order to solve the problem?need to initialize Result:Screen.Recording.2023-06-22.at.7.43.03.AM.movWhat alternative solutions did you explore? (Optional)N/A |
ProposalPlease re-state the problem that we are trying to solve in this issue.Date of birth year deep link does not change the year What is the root cause of that problem?We are replacing the App/src/components/CalendarPicker/index.js Lines 27 to 29 in 9bef507 Also we are using the App/src/pages/YearPickerPage.js Line 72 in fb8ca92 What changes do you think we should make in order to solve the problem?I've noticed that currently currency selector not showing its query string param in the link after selecting the currency, Since year-selector behave similarly, then we can follow the currency selector pattern to achieve the same. We can change What alternative solutions did you explore? (Optional)If we decided not delete the year param from the URL, we can replace the year from the very beginning when we init We can move this code before declaring App/src/components/CalendarPicker/index.js Lines 27 to 29 in 9bef507 And replace the code with Final snippet: |
ProposalPlease re-state the problem that we are trying to solve in this issue.Date of birth year deep link does not change the year What is the root cause of that problem?When selecting a year in YearPickerPage we will set the selected year on the route and then in DateOfBirthPage we will get the selected year from the route to use What changes do you think we should make in order to solve the problem?When selecting a year we should save the selected year on ONYX. To do that we will create a new function to update value on ONYX and using this function when the user selects the year App/src/pages/YearPickerPage.js Line 75 in 9bef507 we should call updateSelectedYear(selectedYear) before navigate back a nd remove year on the route like this Also removing year on route when clicking the back button here App/src/pages/YearPickerPage.js Line 103 in 9bef507 And in DateOFBirthPage we should get selected year from ONYX instead of the route like this What alternative solutions did you explore? (Optional) |
|
Looks like something related to As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our Feel free to drop a note in #expensify-open-source with any questions. |
|
Next steps are to review proposals here |
I needed to close my previous github. This was my proposal and this is my new github. |
|
📣 @jfquevedol2198! 📣
|
|
@allroundexperts thoughts on the above proposals? |
|
Thanks for your proposal @jfquevedol2198 and @wildan-m. But as per this comment, we're looking to NOT save the current year in the route param. Please create new proposals with the result as described here. @joekaufmanexpensify can you please update the OP with the correct expected result? @dukenv0307 Your proposal looks promising but attaching the selected year to Also, I'm not entirely sold on using |
|
Still looking for more proposals. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Date of birth year deep link does not change the year What is the root cause of that problem?
App/src/pages/YearPickerPage.js Lines 66 to 77 in 98bb603 What changes do you think we should make in order to solve the problem?We need to add a function
after change:
const listenersMap = {};
function addListener(eventName, listener) {
listenersMap[eventName] = listenersMap[eventName] || [];
listenersMap[eventName].push(listener);
}
function removeListener(eventName, listener) {
const lis = listenersMap[eventName];
if (!lis) return;
for (let i = lis.length - 1; i >= 0; i--) {
if (lis[i] === listener) {
lis.splice(i, 1);
break;
}
}
}
function removeAllListeners(eventName) {
listenersMap[eventName] = [];
}
function emit(eventName, ...params) {
const listeners = listenersMap[eventName];
if (!listeners) return false;
listeners.forEach(fnc => {
fnc(...params);
});
return true;
}
export default {
addListener,
removeListener,
removeAllListeners,
emit,
};
const onSelectYear = (year) => {
setSelectedYear(year);
}
useEffect(() => {
EventEmitter.addListener('onSelectYear', onSelectYear);
return () => EventEmitter.removeListener('onSelectYear', onSelectYear);
}, []);
after change: updateSelectedYear(selectedYear) {
EventEmitter.emit('onSelectYear', `${selectedYear}`);
Navigation.goBack();
}ResultScreen.Recording.2023-06-26.at.12.54.13.PM.movWhat alternative solutions did you explore? (Optional)N/A |
|
Updated expected behavior in OP! |
|
Pending review of new proposal |
|
Thanks for your updated proposal @jfquevedol2198. I'm not entirely convinced of the event emitter approach that you're trying to adopt. I would much prefer a React way to fix this. |
ProposalPlease re-state the problem that we are trying to solve in this issue.Date of birth year deep link does not change the year What is the root cause of that problem?
App/src/pages/YearPickerPage.js Lines 66 to 77 in 98bb603 What changes do you think we should make in order to solve the problem?Add a callback to handle the new year selection in
after change: const [selectedYear, setSelectedYear] = useState(dobYear);
import React from 'react';
const onSelectYearRef = React.createRef();
function set(onSelectYear) {
onSelectYearRef.current = onSelectYear;
}
function update(year) {
if (!onSelectYearRef.current) return;
onSelectYearRef.current(year);
}
export {set, update};
useEffect(() => {
const onSelectYear = (year) => setSelectedYear(year);
SelectYearAction.set(onSelectYear);
}, [])
after change: updateSelectedYear(selectedYear) {
SelectYearAction.update(selectedYear);
Navigation.goBack();
}ResultScreen.Recording.2023-06-28.at.1.13.32.PM.movWhat alternative solutions did you explore? (Optional)N/A |
|
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
|
@allroundexperts could you please re-review the updated proposals when you have a chance? |
|
@allroundexperts @joekaufmanexpensify this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
|
@joekaufmanexpensify I'll have an update by tomorrow. |
|
Great, thanks! |
|
Pending review of updated proposals. |
|
@allroundexperts bump on reviewing updated proposals when you have a sec? |
|
Ah. Not sure how I missed this. Will put this on top of my plate for today. |
|
Great, ty! |
|
@jfquevedol2198 Thanks for sharing your branch. The issue I can see with your current approach is that it won't be able to handle multiple Year selection fields on the same page. I think we can simplify this problem by using React Contexts. Let me know your thoughts. |
|
@allroundexperts You are right, if we have multiple Year selection fields, it won't work exactly. |
|
@allroundexperts here is the example with multiple Year selection on a page: Screen.Recording.2023-07-11.at.7.56.54.AM.mov |
|
@jfquevedol2198 Thanks for the update. Can you please update your proposal with the new changes as well? |
[Updated Proposal]Please re-state the problem that we are trying to solve in this issue.Date of birth year deep link does not change the year What is the root cause of that problem?
App/src/pages/YearPickerPage.js Lines 66 to 77 in 98bb603 What changes do you think we should make in order to solve the problem?Add a callback to handle the new year selection in
after change: const [selectedYear, setSelectedYear] = useState(dobYear);
import React from 'react';
// store action to handle Year Update
const onSelectYearRef = React.createRef();
// store inputID of DatePicker Form for identifying Form in the case of multiple `DatePicker` on same page
const inputIdRef = React.createRef();
function set(onSelectYear) {
onSelectYearRef.current = onSelectYear;
}
function emit(year) {
if (!onSelectYearRef.current) return;
onSelectYearRef.current(inputIdRef.current, year);
}
function updateInputID(inputID) {
inputIdRef.current = inputID;
}
export {set, emit, updateInputID};
useEffect(() => {
const onSelectYear = (inputId, year) => {
if (inputId !== 'dob') return;
setSelectedYear(year.toString());
}
SelectYearAction.set(onSelectYear);
}, [])
const propTypes = {
/** A string to identify calendar picker */
id: PropTypes.string,
...
}
onYearPickerPressed() {
SelectYearAction.updateInputID(this.props.id);
...
}
after change: updateSelectedYear(selectedYear) {
SelectYearAction.emit(selectedYear);
Navigation.goBack();
}ResultScreen.Recording.2023-06-28.at.1.13.32.PM.movWhat alternative solutions did you explore? (Optional)N/A |
|
Pending review of updated proposal |
|
I'm still not sure if using |
|
📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸 |
|
Sounds good, thanks! |
|
@allroundexperts @joekaufmanexpensify this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks! |
|
I am curious is this still a bug after the Calendar refactor we did here #21792? |
|
Ah, yep good call. I just rechecked now, and it looks like this was fixed by that PR. We no longer specify the year in the link when you change the year of birth in the date picker. So I feel like we can go ahead and close this. Does that sound good to everyone? |
|
Sounds good to me! |
|
Cool cool. Closing! |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
I think it's odd that we change the link from https://staging.new.expensify.com/settings/profile/personal-details/date-of-birth to https://staging.new.expensify.com/settings/profile/personal-details/date-of-birth?year=YYYY when you select a different year of birth (before saving). But we don't change the link at all when adjusting month or day of birth.
IMO, the ideal behavior here is to stop changing the link when you adjust year of birth before saving. And then make it so you can't deep link to change your year of birth at all. This isn't something an actual user will frequently/ever be doing.
Actual Result:
App does not change the year in textbox above datepicker and also does not consider the new year on date selection if we visit modified year deep link
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.3.27-6
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
deep.link.modified.year.not.working.1.mp4
Expensify/Expensify Issue URL:
Issue reported by: @dhanashree-sawant
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1686756962010819
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: